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

feat: adding option_fn field to option_spec #12

Merged
merged 7 commits into from
May 11, 2024
Merged

feat: adding option_fn field to option_spec #12

merged 7 commits into from
May 11, 2024

Conversation

dgkf
Copy link
Owner

@dgkf dgkf commented Apr 22, 2024

Adds option_fn, a function that may be applied to the return value of an option before its value is retrieved.

Capabilities made possible:

  • Configuring an option as a function or quote, but allowing opt() to return the result of calling the function without having to consistently apply the function or wrap it in a specialized helper.
  • Adding side effects to access via opt()
  • Coercing values into a consistent type before being retrieved
  • Specializing error messages when an option does not have an expected format

Behaviors still under consideration:

  • Should this function apply to the default value? For consistency I'm leaning toward yes.
  • The expected signature should certainly accept the value, but beyond that I'm not totally sure. For now (considered experimental in the docs) I pass all relevant info related to retrieving the value, but this feels a bit excessive.

@tdeenes, your feedback would be greatly appreciated if you can take a look

@dgkf dgkf linked an issue Apr 22, 2024 that may be closed by this pull request
"envir" = spec$envvar_fn(Sys.getenv(spec$envvar_name), spec$envvar_name),
"option" = getOption(spec$option_name),
"default" = get_option_default_value(x, optenv),
if (missing(default)) stop(sprintf("option '%s' not found.", x))
else default
)

spec$option_fn(
Copy link
Contributor

@tdeenes tdeenes Apr 22, 2024

Choose a reason for hiding this comment

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

I like the idea that we can pass arbitrary arguments to option_fn via .... However, if we allow for this, two other changes are required:

  1. The doc #' @param ... Additional arguments unused must be updated in line 8;
  2. A more thorough check of option_fn is required, otherwise the user can get weird error messages or even worse, inconsistent failures/wrong results;
  3. Depending on how strict we want to be with the signature of option_fn, we might introduce a little overhead here and check what arguments to pass to option_fn:
.call <- as.call(
  c(
    list(spec$option_fn$fn, value),
    alist(x = x, default = default, env = env, ... = ..., source = source)[spec$option_fn$args]
  )
)
eval(.call)

See my comment at option_spec for the proposed utility function.

R/options_spec.R Outdated Show resolved Hide resolved
R/options_spec.R Outdated Show resolved Hide resolved
Copy link
Contributor

@tdeenes tdeenes left a comment

Choose a reason for hiding this comment

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

Nice! There are a few typos in the doc which shall be fixed. I also added two optional comments on the use of ... in option_fn.

Copy link
Owner Author

@dgkf dgkf left a comment

Choose a reason for hiding this comment

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

Fixing spelling - thanks @tdeenes

Fixing spelling

Co-authored-by: Dénes Tóth <tdeenes@users.noreply.github.com>
@tdeenes
Copy link
Contributor

tdeenes commented Apr 26, 2024

@dgkf I think you forgot to update the roxygen doc:

#' @param ... Additional arguments unused

This should be modified to:

#' @param ... Arguments passed to `option_fn`

@dgkf dgkf merged commit bc66c2f into main May 11, 2024
7 checks passed
@dgkf dgkf deleted the 11-opt-fn branch May 11, 2024 15:45
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.

Add validation capability
2 participants