dev crayon breaks a test case

[gaborcsardi]

In particular:

    Running ‘test-pager.R’ [2s/2s]
    Comparing ‘test-pager.Rout’ to ‘test-pager.Rout.save’ ...421c421
  < [1]  TRUE FALSE FALSE
  ---
  > [1] TRUE TRUE TRUE

I am not sure why it happens. The detection of ANSI support is more detailed in the dev version, maybe that's why.

Also, it only breaks in non-interactive mode.

[brodieG]

Thanks for reporting. I'll check it out in the next week or so.

[brodieG]

This is because the new crayon version changes the semantics of crayon.enabled and crayon.colors. It used to be the case that a NULL crayon.enabled let crayon.colors set colors. Now that is no longer the case, except kind of if you're in RSTUDIO.

Before:

Downloads$ Rscript -e "
> packageVersion('crayon');
> options(crayon.enabled=TRUE);
> options(crayon.colors=8);
> Sys.setenv(RSTUDIO='1');
> crayon::num_colors(TRUE);"
[1] ‘1.3.4’
[1] 8
Downloads$ Rscript -e "
> packageVersion('crayon');
> options(crayon.colors=8);
> Sys.setenv(RSTUDIO='1');
> crayon::num_colors(TRUE);"
[1] ‘1.3.4’
[1] 8

After:

Downloads$ Rscript -e "remotes::install_github('r-lib/crayon')"
Downloading GitHub repo r-lib/crayon@HEAD
...
* DONE (crayon)
Downloads$ Rscript -e "
> packageVersion('crayon');
> options(crayon.enabled=TRUE);
> options(crayon.colors=8);
> Sys.setenv(RSTUDIO='1');
> crayon::num_colors(TRUE);"
[1] ‘1.3.4.9000’
[1] 8
Downloads$ Rscript -e "
> packageVersion('crayon');
> options(crayon.colors=8);
> Sys.setenv(RSTUDIO='1');
> crayon::num_colors(TRUE);"
[1] ‘1.3.4.9000’
[1] 256
Downloads$ Rscript -e "
> packageVersion('crayon');
> options(crayon.enabled=FALSE);
> options(crayon.colors=8);
> Sys.setenv(RSTUDIO='1');
> crayon::num_colors(TRUE);"
[1] ‘1.3.4.9000’
[1] 1

It's debatable whether the previous assumption that is.null(crayon.enabled ) let crayon.colors prevail is good or not, but it doesn't seem obviously wrong. The new behavior does seem a bit odd to me where RSTUDIO overrides a NULL, but crayon.colors does not, yet crayon.colors overrides RSTUDIO if crayon.enabled is TRUE. You can certainly make an argument that this behavior is good, but is it unambiguously better than previous behavior?

The following restores old behavior, and R CMD check passes with it (both for crayon and diffobj):

diff --git a/R/aab-num-ansi-colors.R b/R/aab-num-ansi-colors.R
index 5c7651c..6b7bd4d 100644
--- a/R/aab-num-ansi-colors.R
+++ b/R/aab-num-ansi-colors.R
@@ -99,10 +99,11 @@ num_ansi_colors <- function(stream = "auto") {
   # compatiblity with crayon
   cray_opt_has <- getOption("crayon.enabled", NULL)
   cray_opt_num <- getOption("crayon.colors", NULL)
-  if (!is.null(cray_opt_has) && !isTRUE(cray_opt_has)) return(1L)
-  if (isTRUE(cray_opt_has) && !is.null(cray_opt_num)) {
+  if ((is.null(cray_opt_has) || isTRUE(cray_opt_has)) &&
+      !is.null(cray_opt_num)) {
     return(as.integer(cray_opt_num))
   }
+  if (!is.null(cray_opt_has) && !isTRUE(cray_opt_has)) return(1L)
   if (isTRUE(cray_opt_has) && is.null(cray_opt_num)) {
     return(8L)
   }

Will you consider restoring the treatment of crayon.enabled when it is not set? If you will then I'm happy to PR the above (or you can just apply directly). If not, I'm due for a diffobj submission anyway so it's not the end of the world for me to change my tests, although I do prefer the old semantics.

[gaborcsardi]

This is because the new crayon version changes the semantics of crayon.enabled and crayon.colors. It used to be the case that a NULL crayon.enabled let crayon.colors set colors. Now that is no longer the case, except kind of if you're in RSTUDIO.

Weird, that is probably not what I intended.

[gaborcsardi]

Btw. you cannot simulate RStudio with setting a single env var any more, the detection is more sophisticated now, because env vars are also set in sub-processes of RStudio.

Also, the forget argument of num_colors() is ignored now, but crayon is smarter about what it can cache and what it cannot.

[gaborcsardi]

Re the suggested patch, it is not ideal, because now has_color() is simply num_colors() > 1L, so that always turns colors on if crayon.enabled == NULL && crayon.colors > 1L. Which is not what should happen.

[gaborcsardi]

Btw2. there is now a single env var or option to set the number of colors and whether to use them: cli.num_colors. So to force the number of colors, you just need to set that. crayon.enabled and crayon.colors will be deprecated soon, but will be still around for a very long time. (And you probably still want to set them in the tests for older crayon versions.)

[brodieG]

Re the suggested patch, it is not ideal, because now has_color() is simply num_colors() > 1L, so that always turns colors on if crayon.enabled == NULL && crayon.colors > 1L. Which is not what should happen.

Good point. Maybe add an internal flag to num_colors that lets it ignore crayon.colors if crayon.enabled is NULL? Absent that it seems difficult to have consistent semantics for both has_colors and num_colors across versions since the dependency of the functions has switched (num_colors used to use has_colors, and now it's the opposite).

[gaborcsardi]

I can restore the original behavior without a new flag, no problem.

[gaborcsardi]

OK, https://github.com/r-lib/crayon/commit/b605cb8de202a04ced6a877dbcfae3c411f6598e should fix this. diffobj now checks without errors, so I'll close this issue.

[brodieG]

Great, thank you!