Conversation
There was a problem hiding this comment.
Great work, I really like having facet aesthetics and handle their formatting by scales. Most comments below are nitpicks that you can ignore, but there are a few topics I'd want to raise.
I'm tempted to suggest that SETTING scales => 'free' should live in coords.
Previously the facet writer logic wasn't complicated enough to merit a module on its own, whereas I think it has become too large to live in the writer's mod.rs file and it should live in its own file.
I don't like the RENAMING clause as part of the facet. If we just have it in scales, there is a single source of truth for the renaming which will lead to less confusion because people don't have to think about precedence. Moreover, it currently duplicates a bunch of logic which makes maintenance worse for (in my eyes) identical functionality. Even if you have strong reasons for keeping it, deduplicating the logic may be worth it in the long run.
| free_x: bool, | ||
| free_y: bool, |
There was a problem hiding this comment.
I'm wondering if we shouldn't create another context object to capture additional arguments to build_layers
There was a problem hiding this comment.
yeah, it is beginning to grow out of hand. Do you propose a struct only for free_x/free_y for now? I feel the other arguments are proper first class information
tree-sitter-ggsql/grammar.js
Outdated
| 'fixed', 'free', 'free_x', 'free_y' | ||
| // RENAMING clause for facet strip labels | ||
| // Syntax: RENAMING 'A' => 'Alpha', 'B' => 'Beta', * => 'Region: {}' | ||
| facet_renaming_clause: $ => seq( |
There was a problem hiding this comment.
As before, if we want to keep facet renaming open, can't we re-use the scale_renaming_clause or rename to renaming_clause and use in both places?
|
Just to comment on the two overarching questions. I don't think I have gone back and forth when it comes to removing |
Hmm I hear you, but I think the scope of that use case is relatively narrow. The case where this might be useful is (1) if you use a grid layout and (2) if you have variables that need formatting and (3) both variables should be formatted in the same way. I don't see |
Fix #6
This PR massively rewrites the faceting clause, allowing scaling of faceting variables, mapping inside layers to overwrite the default, repeating of layers missing faceting variables, and a lot of other stuff
The docs are there to describe the new functionality