Skip to content
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

Merged
merged 9 commits into from
Sep 22, 2023
Merged

Refactor options handling #384

merged 9 commits into from
Sep 22, 2023

Conversation

etiennebacher
Copy link
Collaborator

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 becomes maintain_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)

library(polars)

pl$options
#> $maintain_order
#> [1] FALSE
#>
#> $do_not_repeat_call
#> [1] FALSE
#>
#> $strictly_immutable
#> [1] TRUE
#>
#> $debug_polars
#> [1] FALSE
#>
#> $no_messages
#> [1] FALSE
original_options <- pl$options

# this modifies options
pl$set_options(strictly_immutable = FALSE, maintain_order = TRUE)
pl$options
#> $maintain_order
#> [1] TRUE
#>
#> $do_not_repeat_call
#> [1] FALSE
#>
#> $strictly_immutable
#> [1] FALSE
#>
#> $debug_polars
#> [1] FALSE
#>
#> $no_messages
#> [1] FALSE

# if an option is not specified, it's not modified (here, maintain_order is
# still TRUE)
pl$set_options(strictly_immutable = TRUE)
pl$options
#> $maintain_order
#> [1] TRUE
#>
#> $do_not_repeat_call
#> [1] FALSE
#>
#> $strictly_immutable
#> [1] TRUE
#>
#> $debug_polars
#> [1] FALSE
#>
#> $no_messages
#> [1] FALSE

# reset to default
pl$reset_options()
identical(pl$options, original_options)
#> [1] TRUE

@eitsupi
Copy link
Collaborator

eitsupi commented Sep 12, 2023

Thanks!

This looks good to me, but I don't know if this should be preferred over the options function that is common in R.
Or can implement the options function way be built on top of this later?

@etiennebacher
Copy link
Collaborator Author

I don't think options() has autocompletion, which is quite nice in pl$set_options():

image

And I have to think about how annoying it would be to synchronize the options passed to options() and those passed to pl$set_options().

R/options.R Show resolved Hide resolved
@etiennebacher
Copy link
Collaborator Author

I'll try to find an alternative to the unlockBinding() approach since it raises a warning in R CMD check.

@etiennebacher

This comment was marked as outdated.

@etiennebacher etiennebacher marked this pull request as ready for review September 21, 2023 09:06
@etiennebacher
Copy link
Collaborator Author

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

Copy link
Collaborator

@sorhawell sorhawell left a 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 :)

R/expr__expr.R Show resolved Hide resolved
R/options.R Outdated Show resolved Hide resolved
Copy link
Collaborator

@sorhawell sorhawell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great thanks :)

@etiennebacher etiennebacher merged commit 9402b30 into main Sep 22, 2023
10 checks passed
@etiennebacher etiennebacher deleted the refactor-options branch September 22, 2023 13:03
@eitsupi
Copy link
Collaborator

eitsupi commented Sep 22, 2023

Oops, I didn't expect this to be merged before 0.8.1 since it's a breaking change.

@etiennebacher
Copy link
Collaborator Author

Ah sorry, I can try to revert this

@etiennebacher etiennebacher restored the refactor-options branch September 22, 2023 15:31
etiennebacher added a commit that referenced this pull request Sep 22, 2023
@etiennebacher etiennebacher deleted the refactor-options branch September 23, 2023 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants