-
Notifications
You must be signed in to change notification settings - Fork 794
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: Support a wider range of iterables in SchemaBase.to_dict
#3501
Conversation
Previously `list[set[str]]`
AFAIK, this is the intended case for `Parameter | Expression` not for converting arbitrary objects
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.
Looking great overall @dangotbanned , thank you again! A few comments / questions.
Thanks for taking the time to review @joelostblom Will wait for your response before making any changes |
Thanks for responding to the comment @dangotbanned ! At a cursory glance it all looks good but I will review more in detail and try it out when I'm back from vacation in about a week |
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.
Looking great! I found a day to review this more in depth and left a few comments. I should be able to take another look in ~4 days.
As part of this PR, could you also update the docs here https://altair-viz.github.io/user_guide/encodings/index.html#sort-option to not only mention that lists can be passed but any ordered sequence would work? |
The original test obscured the fact that this change applies anywhere a `Sequence` is annotated.
Thanks for the second look @joelostblom I'm going to work through your comments after this, but thought I'd give a heads-up on #3501 (commits) After reading your comments this morning, I got the feeling that the focus on ordering/sets had overlooked the much broader scope of this change. I think this was my mistake by writing tests that only covered
The ordering requirement makes a lot of sense for |
Yeah that sounds like a good idea, thanks @joelostblom I will work on that for the next commit |
…rors` All 18 cases use this, saves 30 lines
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 again for this contribution and your patience with the review process @dangotbanned. I only have a few minor comments/clarifications left and then we should be ready to merge.
Co-authored-by: Joel Ostblom <[email protected]>
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.
Super, thanks for the quick turn around! I'm excited to merge this PR and no longer have to convert series etc to lists explicitly =) Thanks again!
Fixes: #2808 and #2877
Supersedes #3237
CI Failures
See #3501 (comment)
Notes
Huge thanks to @MarcoGorelli as
narwhals
made this feature super simple to implement.In addition to handling
Sequence
like types, anIterator
can safely be passed tonarwhals.from_native
without being consumed.Code block
#2808 Cases
Large screenshots
#2877 Case
Large screenshot
Tasks