-
Notifications
You must be signed in to change notification settings - Fork 367
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
[BREAKING] Make DataFrameColumns stop being an AbstractVector #2291
Conversation
@nalimilan - it should be good to have a look at. |
As usual some corner cases of the changes popped up. |
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; just the use of [begin]
causing 1.0 test failures
Thank you for having a look at it! |
This looks great! |
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
@nalimilan - your questions were exactly the things I had to check against Base when implementing it. These interfaces are really non-obvious. |
So the major point of discussion is this case:
As you can see - I have implemented methods to follow what happens in Base with vectors. We can change it to follow your proposal though. What do you think? |
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
OK, makes sense. Then the only remaining issue to settle is whether to create a |
You convinced me to return a |
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Thank you! |
Fixes #2229
I have added functionalities for
DataFrameColumns
that I find useful, but please comment if you would like to see more.