-
Notifications
You must be signed in to change notification settings - Fork 608
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat!: supporting channel name string for sorting by another encoding (e.g., sort: "x"
, sort: "-x"
)
#5134
Conversation
Interesting point. It's also worth noting that:
That said, as I just saw yesterday that Altair may introduce a chained syntax like
Basically, users will only see examples that show sort strings as channels. They can easily guess that x/y are channels as we never use x/y as the field names in the examples. The document will also states that sort strings are either In a rare case that the user's plot has a field named "y" mapped to x-encoding or vice versa, it will be confusing anyway no matter we introduce this syntax or not. |
I don't have a strong opinion on this. Besides the point @domoritz made of ambiguity between field/encoding, which does seem problematic, I'm also curious whether this introduces multiple ways of specifying sorting or are we getting rid of the longform? If multiple, then is this sufficiently important to warrant the added complexity (code paths/documentation/learnability/discoverability/etc.)? Another way of looking at it is perhaps the new Vega-Lite JS API frees us from optimizing the concision of the JSON for hand-writing use cases (and, instead, the JSON can focus on concision with respect to the grammar and issues of abstraction). The fact that Altair is considering adding a shorthand makes me believe that there is less of a need for the JSON to handle it directly. |
Big +1 on this; it would address Altair feature requests as well: vega/altair#884 |
This is definitely another way of doing things and is the biggest con of this PR IMHO (moreso than the potentil confusion, which I think is not so strong). If we agree that the shorter sort-by-encoding syntax is more desireable, I think we should deprecate the old I haven't deprecated the old syntax in this PR yet, simply for backward compatibility. But I agree we could remove it from docs / schema but maintain the functionality as a macro.
I thought about this too. Having the JS APIs provide concise can be useful in some cases, but doesn't come with a cost. For things that we want the languages to be consistently concise across different wrappers (JS, Altair, and others), it is better to provide some of the concision mechanism in the core JSON syntax, so the syntax will be consistent across wrappers. |
So if we deprecate the object for sorting by channel, we will have three ways to sort: For natural order:
For channels:
For fields:
I think we already introduced a shorthand with For natural order:
For channels:
For fields:
I'm a bit conflicted because the latter is definitely easier to reason about for a machine but the former is easier for people. Right now we are in between two options. I would actually be okay with supporting both and normalize the former to the latter. I am also okay with deprecating the latter. I am also okay with only writing about the former in the docs if we want to keep the latter. |
Regarding the shorthands: what happens if you have a field named |
As mentioned above, we will use strings only for either (1) ascending/descending or (2) encoding channels (with an optional minus prefix) -- but not field names for this exact reason.
I think having 3 ways to sort (for 3 distinct use cases) it not a problem. It's only a problem when we provide two ways for one of the use case. If we deprecate
So the only change would be that Note: FWIW, we currently don't support |
sort: "x"
, sort: "-x"
)sort: "x"
, sort: "-x"
)
Overall, I see that this change is useful for users but sacrifices some easy programmatic handling. I would prefer to see this built into Altair and the VL JS API instead. However, adding it in Vega-Lite directly will increase adoption of the idea. |
3e51fc6
to
49f8d83
Compare
sort: "x"
, sort: "-x"
)sort: "x"
, sort: "-x"
)
So @kanitw changed to support both the shorthand and the expanded version. Internally, we expand in the normalizer. I generally don't like having two ways to do the same thing but in this case I think an exception is warranted as it makes things much easier for users and ensures consistency among programming APIs like Altair, Elm Vega, and the VL API. |
Fix #4933