Index to grouped data frame using Dicts#2281
Conversation
|
Thank you for the fix. The design looks good to me. I have left some comments. Also |
|
As I have commented earlier this https://github.com/JuliaData/DataFrames.jl/pull/2281/files#diff-6349421a054bb7e74b7f27ae86304cbaR342 also should be fixed. |
|
Sorry about that. Was fixing them but got distracted. Should be ready for another review. I added a |
|
I have pushed some small things. If CI passes can you please have a final look at it and confirm all is clear and works as intended? Then I think we can approve it (but I will wait with merging for @nalimilan as he has not commented on this API change). |
nalimilan
left a comment
There was a problem hiding this comment.
Sorry for the slow reaction. Looks mostly good.
|
I've added |
|
CI still fails - can you please check the PR on Julia 1.0 and on Julia nightly to check what causes a problem? Thank you! |
|
Works now! |
|
Bravo! 😄 |
|
OK - so can you please just merge it with master and add NEWS.md entry please? @nalimilan - if you do not comment on this I am going to merge this PR after @pdeffebach adds NEWS.md entry and then work on implementing the conclusions from #2305 including handling |
|
That was a lot easier than a rebase! I think it's ready. |
|
I have resolved merge conflict. |
|
Looks good to merge. @nalimilan - can you please have a quick look? I would merge this after CI passes (probably tomorrow as we have a lot of CI runs queued). |
|
bump 😄 (I would finish it not to have constantly merge it with master) |
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
|
Thanks for pinging me on this! Slipped through. Thanks for the feedback Milan. |
|
also merge conflicts need resolving |
|
Conflict was just |
|
Thank you! |
Following discussion in #2265, this is the better strategy.
This PR so far has the implementation and docs, but no tests.