-
Notifications
You must be signed in to change notification settings - Fork 36
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 options
handling
#384
Conversation
Thanks! This looks good to me, but I don't know if this should be preferred over the |
I'll try to find an alternative to the |
This comment was marked as outdated.
This comment was marked as outdated.
All tests pass now but there are still unresolved comments. @sorhawell @eitsupi if you agree with the changes I'll update the NEWS before merging |
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 will continue review tonight I hope :)
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.
Great thanks :)
Oops, I didn't expect this to be merged before 0.8.1 since it's a breaking change. |
Ah sorry, I can try to revert this |
This reverts commit 9402b30.
Related to #316. @sorhawell I just made this to show with examples what I’d like to do. I didn’t check tests yet, it’s mostly to discuss about how to handle options.
There are 3 methods related to options:
pl$options
shows a named list with options and their default values,pl$set_options()
modifies the values but only for the arguments that were specified in the call (and it works with autocomplete),pl$reset_options()
brings the options back to their default.I also used this occasion to rename some options (
default_maintain_order
becomesmaintain_order
).I think this is clearer for the user and it also gets rid of a lot of internal code, what do you think? (also curious about your opinion @eitsupi)