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

Use internal default as fallback for global options #160

Merged
merged 1 commit into from
Sep 27, 2021

Conversation

gadenbuie
Copy link
Contributor

I ran into this issue in a situation where we're evaluating semi-trusted user input where we want to be careful that the user cannot change external settings. As a result, we're carefully scrubbing and restoring the state of global option after evaluating the user input. Unfortunately, this led me to an issue where diffobj (and also fansi) set global options on load that may later removed and break package functionality.

Here's a simple reprex. Run a function that attaches diffobj, but where options are protected.

old <- options()
waldo::compare(1:5, 3:7)
#> `old`: 1 2 3 4 5
#> `new`: 3 4 5 6 7

Yes, the options are protected a little aggressively, restoring unset options to their original state...

nulls <- setdiff(names(options()), names(old))
old[nulls] <- list(NULL)
options(old)

with the side-effect of removing the diffobj.max.diffs option...

options()$max.diffs
#> NULL

breaking any subsequent call to ses().

waldo::compare(1:5, 3:7)
#> Error in ses_prep(a = a, b = b, max.diffs = max.diffs, warn = warn): Argument `max.diffs` must be scalar integer.

(Something similar happens for diffobj.warn.)

The solution I'm proposing is to fall back to the internal diffobj default value in gdo(). I think this is reasonable since I think it doesn't change the intended behavior and it matches the expectations described in the documentation:

#' @param warn TRUE (default) or FALSE whether to warn if we hit
#'   \code{max.diffs}.

I recognize I haven't completely though through all of the implications, if there are any broader problems please feel free to treat this PR as an issue report 😄

@brodieG
Copy link
Owner

brodieG commented Sep 23, 2021

Garrick, thanks for reporting this issue. I agree your proposed solution is better behavior in all cases. I'll need to review a little more closely, and might ask you to submit the patch against the development branch (although please wait as it is currently out of date...).

@brodieG
Copy link
Owner

brodieG commented Sep 27, 2021

Garrick, this looks good to me. Will you change the target branch to the development branch? Also, if you're up for it go ahead and add a NEWS entry.

@brodieG brodieG changed the base branch from master to development September 27, 2021 10:38
@brodieG brodieG merged commit 7d9cedf into brodieG:development Sep 27, 2021
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.

2 participants