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

Unexpected behavior of opts #13

Closed
tdeenes opened this issue Apr 23, 2024 · 2 comments · Fixed by #17
Closed

Unexpected behavior of opts #13

tdeenes opened this issue Apr 23, 2024 · 2 comments · Fixed by #17
Labels
bug Something isn't working
Milestone

Comments

@tdeenes
Copy link
Contributor

tdeenes commented Apr 23, 2024

When calling opts(xs = NULL) , the function returns an options_list, which means the default values (passed as default= during declaration) are returned.
E.g.:

define_option(option = "test", default = "original_value")
stopifnot(identical(opts()$test, "original_value")

When calling opts(sx = "test"), the current (evaluated) value(s) of the xs option(s) are returned. This behavior leads to confusion when one updates the value of an option:

opt_set("test", "new_value")
## opts.NULL() returns the value at declaration
stopifnot(identical(opts()$test, "original_value"))
## opts.character returns the current value
stopifnot(identical(opts("test")$test, "new_value"))

I would argue opts(xs = NULL) should also return the current evaluated values, but for backward compatibility, we can leave it as it is and define a new method for logical xs.

#' @param xs a boolean flag (`TRUE` = all, `FALSE` = none) or a named logical vector indicating which options shall be evaluated instead of returning its default value
#' @export
opts.logical <- function(xs, env = parent.frame()) {
  env <- get_options_env(as_env(env), inherits = TRUE)
  out <- as_options_list(env)
  if (length(xs) == 0L) return(out)
  is_named <- list_is_all_named(xs)
  stopifnot(
    "'xs' must be named if not a single boolean value" = is_named ||
      length(xs) == 1L
  )
  if (any(xs)) {
    out[xs] <- lapply(names(out)[xs], opt, env = env)
  }
  out
}

I also noticed that opts is a generic without ... in its signature (hence, the methods miss ellipses, too). Is this intentional?

@dgkf dgkf added the bug Something isn't working label Apr 24, 2024
@dgkf
Copy link
Owner

dgkf commented Apr 24, 2024

Thanks for reporting this! Yes, this is very unintuitive. I agree, the NULL value should be equivalent to passing all the names of the defined options. I consider this a bug.

but for backward compatibility

Since the package hasn't yet hit v1.0.0, I'm partial to allowing breaking changes if it helps simplify the interface.

I also noticed that opts is a generic without ... in its signature (hence, the methods miss ellipses, too). Is this intentional?

Not intentional, and is a good practice I should adopt here. I didn't really imagine that other methods would need to be defined, but I don't feel the need to close any doors on anyone that finds some clever use for other methods.

@dgkf dgkf added this to the 0.2.0 milestone Apr 24, 2024
@tdeenes
Copy link
Contributor Author

tdeenes commented Apr 24, 2024

Let me know if I can help with any of these issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants