Use fallback value for critical unset global options

[gadenbuie]

In nchar_ctl() (and other places) TRUE is intended to be the default value of warn.

fansi/R/substr2.R

Lines 98 to 101 in d872ac0

	#' @param warn TRUE (default) or FALSE, whether to warn when potentially
	#' problematic _Control Sequences_ are encountered. These could cause the
	#' assumptions `fansi` makes about how strings are rendered on your display
	#' to be incorrect, for example by moving the cursor (see [fansi]).

fansi/R/nchar.R

Lines 62 to 65 in d872ac0

	nchar_ctl <- function(
	x, type='chars', allowNA=FALSE, keepNA=NA, ctl='all',
	warn=getOption('fansi.warn'), strip
	) {

It's expected that getOption("fansi.warn") will yield TRUE, as set by fansi on load. But this option might inadvertently be unset, causing nchar_ctl() to throw an unexpected error. (See brodieG/diffobj#160 for a more realistic example than the one below.)

# imagine fansi is loaded elsewhere but its expected options are reset somehow
options(fansi.warn = NULL)

waldo::compare(1:5, 3:7)
#> Error in fansi::nchar_ctl(x): Argument `warn` must be TRUE or FALSE.

It would be helpful to declare the fallback default in the function signature with warn = getOption("fansi.warn", TRUE). I could probably help with a PR if you're open to the idea.

[brodieG]

Yes, indeed. A long time ago I chose not to use the default value for getOption for reasons that I've forgotten about. I think it carried over from diffobj, where I had a concern about the very large number of options-based parameters might start incurring a performance penalty (getOption(x) is faster than getOption(x, default) in particular when the option is set, although it is a small difference). I think that was a debatable choice even for diffobj given how much additional overhead is in the functions, and even though fansi is much leaner, the performance cost is still likely negligible overall.

With newer parameters I started switching to including the default there instead of doing it via the .onLoad, and I guess was too lazy to fix the old ones.

All this to say I think changing all the option based parameters to have fixed defaults is a good idea. I'll be happy to review a patch if you want to provide it (but please do so off the development branch). The patch should update all the parameters, and remove the relevant portions in .onLoad. Additionally, it would make sense to add helper functions to set the more complex parameters so they don't make the function signatures overly large (e.g. default_term_cap(), default_css(), or some such).

Alternatively you could wait for me to get to it on my own. I have a big release in the works so it wouldn't be a big deal for me to do that work, and I'm probably not publishing to CRAN either way until I complete the big release.

Thanks for reporting the issue.

[gadenbuie]

Thanks for your reply! I tend to live in a happy world where I don't need to benchmark often, but I tried out a few variations of getOption(opt, default) and (effectively) options()[[opt]] %||% default and I think the results are encouraging for getOption(opt, default).

I appreciate your taking the time to sketch out the work to update. Since the update is a little more involved to get right for fansi, I think I'll wait for now and let you handle this in your next release. It's also not super pressing for my needs now that I know I need to ensure the option is set.

options(beep = "boop")

opts_default <- function(opt, default = NULL) {
  opt <- options()[[opt]]
  if (is.null(opt)) default else opt
}

res <- bench::mark(
  `getOption, set, no fallback`        = getOption("beep"),
  `getOption, set, fallback not used`  = getOption("beep", "boop"),
  `getOption, unset, no fallback`      = getOption("zip"),
  `getOption, unset, fallback used`    = getOption("zip", "zap"),
  `options()$, set, no fallback`       = options()$beep,
  `options()$, set, fallback not used` = opts_default("beep", "boop"),
  `options()$, unset, no fallback`     = options()$zip,
  `options()$, unset, fallback used`   = opts_default("zip", "zap"),
  check = FALSE
)

knitr::kable(res[c(1:5)])

expression	min	median	itr/sec	mem_alloc
getOption, set, no fallback	1.21µs	1.56µs	561975.248	0B
getOption, set, fallback not used	1.34µs	1.61µs	499212.313	0B
getOption, unset, no fallback	1.16µs	1.33µs	608773.485	0B
getOption, unset, fallback used	1.32µs	1.5µs	529824.567	0B
options()$, set, no fallback	138.23µs	151.97µs	6328.956	5.06KB
options()$, set, fallback not used	141.65µs	143.99µs	6548.312	5.06KB
options()$, unset, no fallback	138.7µs	142.88µs	6821.244	5.06KB
options()$, unset, fallback used	139.08µs	143.51µs	6715.060	17.56KB

^{R 3.6.3 on OS X 11.5.2, x86_64-apple-darwin15.6.0 (64-bit)}

[brodieG]

Thanks for running the benchmarks. Release is probably a month or two away.

[brodieG]

This should be fixed on the development branch. LMK if it doesn't work for you. Release is probably still 2-3 weeks away.

[brodieG]

My release estimate was pretty optimistic. I now think I'm 1-2 weeks away, absent any horrors in revdep checks.