posit-dev / positron

Positron, a next-generation data science IDE
Other
1.51k stars 43 forks source link

R or maybe console: ANSI not being stripped correctly for R package snapshot tests #1032

Closed juliasilge closed 10 months ago

juliasilge commented 11 months ago

When we develop R packages, we use snapshot tests via testthat. They are set up so that somehow (???) they print out in the console with nice color formatting but that formatting is stripped to plain markdown in the snapshot .md files. In Positron, that stripping of the formatting is not happening right now:

Screenshot 2023-08-10 at 1 44 42 PM

This is going to be a pretty significant blocker for R package development, because you can't use your snapshots. This all magically working well has been opaque to me in RStudio, so I'm not sure where or how this is solved.

juliasilge commented 11 months ago

I am guessing related to this, when I knit .Rmd, ANSI text (from the cli package here) is not correctly stripped for markdown output:

Screenshot 2023-08-10 at 2 39 32 PM

I made this file via devtools::build_readme().

juliasilge commented 11 months ago

I am working on a package release right now, and this issue is the biggest thing that I had to go back to RStudio for (to run devtools::check(remote = TRUE, manual = TRUE) in my checklist).

lionel- commented 11 months ago

It might be caused by this:

# Legacy option
local({
  withr::local_options(crayon.enabled = FALSE)
  cli::num_ansi_colors()
})
#> [1] 256

# Modern option
local({
  withr::local_options(cli.num_colors = 1)
  cli::num_ansi_colors()
})
#> [1] 1

The first call with the legacy option, which might still be used in some parts of our stack, should return 1 as well.

Looking at the source of num_ansi_colors(), I think the faulty behaviour is caused by this environment variable which takes precedence over the crayon.enabled option:

Sys.getenv("R_CLI_NUM_COLORS", "")
#> [1] "256"

I just tested this and unsetting this envvar seems to solve the problem with devtools::build_rmd().

It is set on startup at: https://github.com/rstudio/positron/blob/0af214efcab041b025dff1c950effbffd596c30d/extensions/positron-r/src/kernel.ts#L255

lionel- commented 11 months ago

oh actually it's probably more about the "knitr.in.progress" branch being overridden by the envvar. If we set an option instead, we'll get the same behaviour.

This reminds me of a discussion I had with Gabor about providing a way for IDEs like ESS to express the number of colours they support by default without affecting other overrides. I think we need either such a mechanism that we could use to add support for Positron, or specific Positron support in cli.

juliasilge commented 10 months ago

This also impacts reprexes, which pretty much work otherwise:

library(parsnip)

## note that `bart()` has a pretty nasty namespace collision:
tidymodels::tidymodels_prefer()

spec <- bart(mode = "regression")
fitted <- fit(spec, mpg ~ ., mtcars)

predict(fitted, new_data = head(mtcars, n = 3))
#> # A tibble: 3 × 1
#>   .pred
#>   <dbl>
#> 1  20.8
#> 2  20.6
#> 3  24.6
predict(fitted, new_data = head(mtcars, n = 3))
#> # A tibble: 3 × 1
#>   .pred
#>   <dbl>
#> 1  20.7
#> 2  20.5
#> 3  24.7
predict(fitted, new_data = head(mtcars, n = 3))
#> # A tibble: 3 × 1
#>   .pred
#>   <dbl>
#> 1  20.8
#> 2  20.6
#> 3  24.6

Created on 2023-08-16 with reprex v2.0.2

lionel- commented 10 months ago

After discussion with @gaborcsardi, it turns out we already had implemented the relevant option: https://github.com/r-lib/cli/issues/398. It just needs this change to be compatible with positron: https://github.com/r-lib/cli/pull/625

So setting options(cli.default_num_colors = 256) on startup with the dev version of cli should fix it.

This won't fix subprocess jobs though because there's no easy way to set options there, so we'll need to revisit when we add a build pane. A possible solution for this case would be a similar environment variable that takes a ppid (parent id of the process) so that cli can match direct subprocesses but not grand children:

CLI_DEFAULT_NUM_COLORS="ppid:nnn;num:256"
juliasilge commented 10 months ago

With the current release version 2023.08.0 (Universal) build 242, I can run snapshot tests via devtools::test(), generate a README via devtools::build_readme(), and make reprexes:

library(parsnip)

## note that `bart()` has a pretty nasty namespace collision:
tidymodels::tidymodels_prefer()

spec <- bart(mode = "regression")
fitted <- fit(spec, mpg ~ ., mtcars)

predict(fitted, new_data = head(mtcars, n = 3))
#> # A tibble: 3 × 1
#>   .pred
#>   <dbl>
#> 1  20.9
#> 2  20.6
#> 3  24.7

predict(fitted, new_data = head(mtcars, n = 3))
#> # A tibble: 3 × 1
#>   .pred
#>   <dbl>
#> 1  20.8
#> 2  20.5
#> 3  24.7

predict(fitted, new_data = head(mtcars, n = 3))
#> # A tibble: 3 × 1
#>   .pred
#>   <dbl>
#> 1  20.8
#> 2  20.6
#> 3  24.8

Created on 2023-08-25 with reprex v2.0.2

(Just as a heads up, the reprex "view" shows up in a browser window, which is different from RStudio but currently expected.)