-
Notifications
You must be signed in to change notification settings - Fork 323
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
Refactor table.group_by to table.aggregate #3339
Conversation
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.
Looks good to me.
I really like how the unit tests already show how this new design seems to be more versatile and readable, even though I still find it a bit peculiar :)
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.
Oh I missed something important.
The unit tests are missing any examples when the grouping happens over keys of more than one field. I think this should be checked - at least for 2-3 fields in the grouping. Also would be good to have at least some tests where the key fields are not first on the list - I know looking at the implementation this probably won't break - but good to check such a case since it is allowed and supported - at least to catch any future regressions and act as documentation of intent.
7f35c3d
to
8a76432
Compare
add_row key = | ||
add_row _ = |
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.
If the key
argument is unused, maybe worth just making it take Nothing
?
Well, technically even, just making it:
add_row =
... the code ...
...
row_index = map.get_or_else key add_row
should work because this will be a no-argument function.
8a76432
to
7f3a683
Compare
Pull Request Description
Following UX work move to
table.aggregate
function.Checklist
Please include the following checklist in your PR:
./run dist
and./run watch
.