r-lib / cli

Tools for making beautiful & useful command line interfaces
https://cli.r-lib.org/
Other
655 stars 70 forks source link

Make `x-r-whatever`-style hyperlinks configurable #739

Closed jennybc closed 2 weeks ago

jennybc commented 4 weeks ago

Relates to https://github.com/posit-dev/positron/pull/5231

This is about making cli hyperlinks work when positron-r carries out package dev tasks, such as devtools::test() in an integrated terminal. An extension can contribute a URI handler, but only URIs with a very specific format are routed into the handler.

gaborcsardi commented 4 weeks ago

Looks good to me'

jennybc commented 2 weeks ago

OK @gaborcsardi this has been promoted to a real PR now. Will you re-review? I think it's close.

gaborcsardi commented 2 weeks ago

Do I understand it correctly that you are going to set the env vars to turn on hyperlinks in Positron tests? If yes, then is it going to cause problems that the env vars will be inherited in subprocesses, and those will also produce hyperlinks? E.g. if a test case knits an Rmd file, that'll potentially have ANSI escapes in the output?

jennybc commented 2 weeks ago

Do I understand it correctly that you are going to set the env vars to turn on hyperlinks in Positron tests? If yes, then is it going to cause problems that the env vars will be inherited in subprocesses, and those will also produce hyperlinks? E.g. if a test case knits an Rmd file, that'll potentially have ANSI escapes in the output?

Yes, I intend to set these env vars when Positron runs tests in a separate R process, i.e. not in the R console. So this would apply to the command "R: Test R Package", which runs devtools::test() and is typically hooked up to Ctrl/Cmd + Shift + T. The goal is to make the usual testthat affordances like testthat::snapshot_accept() and testthat::snapshot_review() work (be clickable). (File hyperlinks will be handled in a separate PR.)

Is it going to cause problems? Well, I think the problems are solvable if it does and are problems worth solving. I.e. it's a matter of test hygiene.

cli itself passes its own tests, with this PR, with Positron's "R: Test R Package" command. And I assume that cli is the package most likely to experience problems.

I did have to add local_clean_cli_context() to 1 test:

https://github.com/r-lib/cli/blob/ef04608e7cdc996b47d738f2886fcac62cb89c95/tests/testthat/test-ansi-hyperlink.R#L243

And this issue is why the new options/env vars have been added to test_that_cli() and local_clean_cli_context(). So the tools are in place for people ensure a "plain vanilla" environment for a test.

jennybc commented 2 weeks ago

I just re-verified that cli passes its own tests, installed from this PR, inside a dev build of Positron, in which I'm setting these new env vars to make x-r-* hyperlinks functional in package dev tasks.

But there's one worth discussing:

R_CLI_HYPERLINK_MODE: "posix"

I wanted to set this to "posix", but that really does cause massive test failure, so I backed off.

The OSC 8 hyperlink "spec" says that ESC \ is the preferred string terminator (which is what "posix" produces here), but that BEL (or \a) is another, less preferred variant. Do you have any interest in moving that direction? I guess it really doesn't matter as long as VS Code / Positron continues to support BEL, which it currently does.

gaborcsardi commented 2 weeks ago

If you are fine with BEL, I don't really mind.

jennybc commented 2 weeks ago

I think this can be merged, but I've identified one problem:

If an expect_snapshot() fails inside test_that_cli(), the hyperlink format is not being taken from the environment in which devtools::test() is being run. Instead, it seems to come from the environment inside the test. In practical terms, it means the advice to run testthat::snapshot_review() is not (yet) a clickable hyperlink for this special case.

I'm struggling to understand why this happens, given that such hyperlinks do appear even when test_that_cli(links = "none"). I.e. hyperlinks can be disabled inside a test, but a failure still leads to a hyperlink (it's just got the wrong format).

If you don't immediately see whatever it is that I'm missing, I propose we merge this so I can move forward in Positron. Then we can return to the matter, once you can see exactly what I'm talking about using a daily build.