Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(partition): legend hover options #978
feat(partition): legend hover options #978
Changes from 34 commits
4016fb0
9d5c4ba
8bc5182
bdf84e9
a3a0842
527baf5
616e70e
5becba9
752cbfb
90f3900
d5e2645
9a19947
d4e592f
9b75150
bdb6cb3
d49528b
47bfe57
8418941
f50ffb5
b5be2e1
6262d2e
1f36925
beebcea
fb25f8f
ebbd65f
b5acaf3
5d69d93
6a6ae39
0300e6a
8e6bd45
43c57b7
a53dd28
5ec5535
35400a1
fa82675
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've noticed this a lot lately in your PRs so I might as well ask here.
Specifically around the
[SOME_KEY]: value
thing...Was this a remnant of javascript? I wonder what the utility is for having the keys extracted like this if they are not shared across different types.
I think typescript will protect against bad keys and this seems overly verbose for no real gain.
@markov00 Thoughts? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can replace all occurrences of these with the values here, with no change whatsoever to runtime or static type check, and the motive isn't that it's remnant of JavaScript, iirc the code was TS off the bat anyway.
The benefit is the following. Certain generic data processing functions, eg.
groupByRollup
, are domain independent, ie. they're not semantically coupled to even data visualization. We might as well import such functions from a 3rd party utility, or we might consider extracting some generic data processing and other utilities into a separate package; even if we never want to do this, it's good practice to not lock in accidental things such as property names.So, not locking in particular field names is just a way of avoiding tight coupling between the data processing functions and their uses. Now it'd be fairly easy to use whatever field names.
It doesn't currently yield some kind of monetary or other short term benefit, though we could move such very generic functions into the collection of relational operators, so I think it's OK as it's not costly to keep it around this way. Btw. tree processing in partition charts may be the only such area that has this kind of keying.
Still, we can discuss removing them, probably for its own PR if it falls that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot about something else for the pro side: if we use literal property names eg.
thing.path
, then the key name occurrence is spread over the code base, and it's hard to find all occurrences.number
is just like anothernumber
) and can be too many indirections even when it worksIn contrast, with the square bracket notation, you use your favorite shortcut to jump to the definition of the key eg.
export const PATH_KEY...
and you use another shortcut, and you get a list of all places where that property is used in the codebase, and nothing more. It's a bit like with functions: you can always jump to the function definition, and to the places where you use the function, unambiguously.In theory.
Currently, we don't gain a lot of this benefit in
groupByRollup
where it has been around since inception, and there's occasional direct use eg.thing.parent
etc. so it's watered up. It could be easily ruled out by using symbols, or symbol.for, but it's not worth it forgroupByRollup
.In sum, it has some benefits, and minor runtime costs too, and it can go either way for
groupByRollup
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the thoughtful explanation.
I see some benefit but imagine doing this for all interfaces across the codebase for better searchability. That would be ridiculous. I think there could be a balance where it's beneficial.
As far as grepping issue, I think if we all be more strict on typing everything, this issue would go away and the code would be better for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. In current
master
, the main (or only?) place is the data transforms ingroup_by_rollup.ts
and that has been that way it was merged over a year ago, so its use isn't growing afaik.The issue with relying on current TypeScript for this is that TS typing is structural (not really, but close enough for this point), and it's always possible for some places to redundantly define something, as long as the prop exists, and is the same name and value type, TS would have no complaint. With the move to nominal typing, we'll be able to have assurance.
So, it'd look unusual if most of our code were full of square brackets for property names, but the technique may be fine for the few generic data processing utilities we have and that we might add to. We might want to deliberate it together and maybe even remove the current square brackets too.
Currently, most of our code, even where logic is quite general, is tightly bound to how we use it, in which case it'd be pointless indeed.
However, successful dataviz and data processing libraries such as D3 are doing something similar: you can specify accessor functions, as a way to tell your D3 function, which object property to reach for, as it may be some arbitrary dataset. Bostock wrote on it here
The named object properties a much more compact, lighter weight, less opinionated way of doing the same thing, eg. it doesn't rely on the module pattern ie. closures with getters/setters that underpins D3; it also doesn't rely on heavier weight constructs eg.
class
, or passing property names explicitly for each function.So, in short, I believe that:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Since this is a constant value that is used here and exported we can use all uppercase for the name for consistency with the rest of the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fa82675