-
-
Notifications
You must be signed in to change notification settings - Fork 35
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(groupbyable)!: simplify implementation and update typing info #207
refactor(groupbyable)!: simplify implementation and update typing info #207
Conversation
Thanks looks good overall !!! I'll do a in-depth review tonight. |
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.
In overall, I'm ok with this PR, it was something that I had in mind, but somehow I was always bouncing it back to the bottom of the tasks list.
There is one thing that needs to be done, is to update the documentation API (api.rst), there is not that much to do:
- Update the
groupBy
signature - Add comprehensive code examples in a sample code file
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.
LGTM, I'll wait a bit so @AlexandruGG can make same feedback, if not, I'll merge it on Monday.
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 like the simplification, thank you @Lctrs!
Please see the comments however, I think we are potentially introducing a bug if we remove the handling of null
values from the callback
* @template NewTKey | ||
* | ||
* @param callable(T=, TKey=): NewTKey $callable |
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 think the only way to prevent non-null values for NewTKey
is to restrict the type of key that can be used here. For example:
@template NewTKey of array-key
@template NewTKey of int|string
It's not ideal because Collection supports using anything as a key due to Generators. So I think the alternative is to just make sure to handle null
properly in the implementation
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 noticed this thing, but I was more keen to let user provide a valid callback.
If the user provide an invalid callback, then the user will get an invalid return.
Maybe we could also improve the operation documentation for 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.
The issue is that nothing in the function signature tells me that null
is invalid there. So in that case I will assume it's supported.
The way I see it, there are a few choices here:
- Continue supporting it like in the current implementation
- Don't support it by updating the type annotations to disallow it
- Don't support it by throwing an exception, but leaving the type annotations as they are so as to allow any key type
- Don't support it by returning an unexpected result
Currently we are doing option 4 which I think is the worst out of these from the user experience perspective
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.
Fair enough, you're right.
Here's my thoughts:
- Option 1: We might end up with an ugly implementation - like the current one on
master
branch, - Option 2: I like this idea, but how would you do that?
- Option 3: I also like this idea,
- Option 4: disqualified!
@Lctrs WDYT ? What's your point of view?
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.
Option 2 is the best. But since it's not possible, it will be option 3.
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.
Yes! This is how I would have implemented it.
Now that I'm reading it, should we also check that the returned value is either int|string
?
WDYT ?
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.
Ooooh that's right. We're working with arrays here. We could just go with @AlexandruGG first proposition here (ie. @template NewTKey of array-key
). We could then widen the possible types later if someone comes with an algo that allows to work with iterable compatible key types.
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 that's even better !!!
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.
Done.
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.
Good for me.
Let's wait for @AlexandruGG 's feedback as he was the initiator of the request.
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.
Nice one, thank you for the good work here @Lctrs 💯
Thanks indeed :) |
This feature is now shipped in version 6.0.0 ! Congrats for your contribution @Lctrs ! |
This PR:
GroupBy
implementations to ease useGroupByable
operations static analysisFollows #206.
Closes #206.