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

jennybc · 2024-10-31T04:06:32Z

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 · 2024-10-31T06:17:32Z

Looks good to me'

jennybc · 2024-11-08T00:31:56Z

R/ansi-hyperlink.R

+get_config_chr <- function(x, default = NULL) {
+  opt <- getOption(paste0("cli.", tolower(x)))
+  if (!is.null(opt)) {
+    stopifnot(is_string(opt))
+    return(opt)
+  }
+
+  env <- Sys.getenv(paste0("R_CLI_", toupper(x)), NA_character_)
+  if (!is.na(env)) return(env)
+
+  default
+}


For everything I've touched, configuration is possible:

Via an option or an env var. Context: in the Positron R console, options are easier for us (or at least that was true at some earlier point? but I have no reason to believe anything has changed). However, for R package dev tasks that run in a separate R process in an integrated terminal, env vars work much better.

The option is consulted first. There was precedent for this.

vignettes/cli-config-user.Rmd

jennybc · 2024-11-08T00:39:29Z

vignettes/cli-config-user.Rmd

+positron://positron.positron-r/cli?command=x-r-run:{code}
+```
+
+(For backwards compatibility with older versions of RStudio, in some contexts, a legacy format is used, e.g. `ide:run:{code}`.)


Do I have this right?

I am not sure what you mean. Don't we always use ide::run in RStudio?

Don't we always use ide::run in RStudio?

I am finding it hard to actually figure this out. The RStudio detection logic is pretty complicated.

I thought there was some plan to start using the x-r-* format in RStudio as well and to phase out the legacy format.

So I'm thinking of this:

rstudio/rstudio#12084

rstudio/rstudio#12096

vignettes/cli-config-user.Rmd

jennybc · 2024-11-08T00:42:13Z

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

gaborcsardi · 2024-11-08T09:08:00Z

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?

gaborcsardi

Thanks! The tests pass, so I am fine with this. :)

jennybc · 2024-11-08T18:29:51Z

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:

cli/tests/testthat/test-ansi-hyperlink.R

Line 243 in ef04608

local_clean_cli_context()

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 · 2024-11-08T20:21:20Z

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 · 2024-11-08T20:26:22Z

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

jennybc · 2024-11-09T01:39:35Z

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.

…ks (#5231) Addresses #5218 This PR makes suggested code hints like `testthat::snapshot_accept()` or `testthat::snapshot_review()` clickable and runnable from the integrated terminal where positron-r sends package development tasks, such as `devtools::test()`. "Runnable" in the sense that the code is run in the user's R console. This gets routed into the same handlers as clickable code entered directly in the console, so the existing safety guards are in place. <img width="544" alt="Screenshot 2024-11-18 at 1 04 51 PM" src="https://github.com/user-attachments/assets/ddead5f3-ee5c-4f36-a50b-12f5ac7e6465"> (Note that this PR does _not_ change behaviour for file hyperlinks, which I've crossed out in the screenshot above. Those hyperlinks currently get delegated to the operating system, so for many R users, a `.R` file will open in RStudio. Changing that is a separate problem #5409.) ### QA Notes To see the new behaviour, you need a development version of the cli R package. I would install that via `pak::pak("r-lib/cli")`. (Context: r-lib/cli#739.) Then you need to run tests or `R CMD check` on a package with a failing snapshot test, in order to get a clickable invitation to accept a new snapshot or to review the proposed snapshot in a Shiny app. I have made a toy package that could be obtained via [`usethis::create_from_github("jennybc/clilinks")`](https://github.com/jennybc/clilinks). It has a couple of snapshot tests that will always fail 😄 because they attempt to snapshot a random number. * Install dev cli: `pak::pak("r-lib/cli")` * Identify a package that tickles this feature and open it in Positron, perhaps via: `usethis::create_from_github("jennybc/clilinks")` * Use our gesture for running package tests: Ctrl/Cmd + Shift + T or select *R: Test R Package* from the command palette. * Click on a "runnable code" hyperlink: - Note this requires Ctrl/Cmd click! This is a VS Code convention, not specific to us. - Note that you might have to click twice. This is a bug (or 2 bugs?) for which fixes are making their way through the system (microsoft/vscode#230010). Again, not us. - You will need to grant the extension permission to handle such URIs, either as a one-off or more persistently. This is a VS Code feature. <img width="544" alt="Screenshot 2024-11-18 at 1 04 51 PM" src="https://github.com/user-attachments/assets/24a880e0-9ebe-4d88-8051-f859480b04fa"> <img width="285" alt="Screenshot 2024-11-18 at 12 55 37 PM" src="https://github.com/user-attachments/assets/d1187098-6a76-44b5-b5a5-be8752c43e5f"> If you click `testthat::snapshot_review()`, you should see the code execute in the R console and this Shiny app appears. Click on "Accept" (the fact that "Skip" seems broken is r-lib/testthat#2025 and unrelated to this PR). <img width="780" alt="Screenshot 2024-11-18 at 12 55 47 PM" src="https://github.com/user-attachments/assets/f7dff7ea-1bf4-4e52-8eb5-e92fd3481cda">

Make .run hyperlinks configurable

5ad1ee2

jennybc added 2 commits October 31, 2024 19:23

Get rid of browser call

69180b8

Handle all the x-r-whatever links

85578b1

jennybc changed the title ~~Make .run hyperlinks configurable~~ Make x-r-whatever-style hyperlinks configurable Nov 2, 2024

jennybc added 10 commits November 1, 2024 17:21

Whitespace

4fe8bd6

Fortify another snapshot test where width can bite you

5ecf351

Oops, level up the .run treatment

74f869a

Test get_config_chr()

74abfb7

Switch vignette links over

1c2a4ff

Test that custom formats come through

5d1f44c

Test custom formats are used

3818ada

Document the env vars

7646e23

Document the options

f6eec6a

Create a section for hyperlinks that covers options and env vars

ef04608

jennybc commented Nov 8, 2024

View reviewed changes

vignettes/cli-config-user.Rmd Show resolved Hide resolved

jennybc marked this pull request as ready for review November 8, 2024 00:41

gaborcsardi approved these changes Nov 8, 2024

View reviewed changes

Remove sidebar to remind me to discuss the "S"

9909597

The options control whether hyperlinks are enabled are not

8ef3a37

gaborcsardi merged commit 169949c into main Nov 9, 2024
13 checks passed

jennybc deleted the positron-arms-length-task branch November 11, 2024 21:20

jennybc mentioned this pull request Nov 13, 2024

Bump dev version and add NEWS bullet #741

Merged

jennybc mentioned this pull request Nov 18, 2024

(Start to) handle cli hyperlinks in the terminal used for pkg dev tasks posit-dev/positron#5231

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jennybc commented Oct 31, 2024 •

edited

Loading

gaborcsardi commented Oct 31, 2024

jennybc Nov 8, 2024

jennybc Nov 8, 2024

gaborcsardi Nov 8, 2024

jennybc Nov 8, 2024 •

edited

Loading

jennybc Nov 8, 2024

jennybc commented Nov 8, 2024

gaborcsardi commented Nov 8, 2024

gaborcsardi left a comment

jennybc commented Nov 8, 2024 •

edited

Loading

jennybc commented Nov 8, 2024

gaborcsardi commented Nov 8, 2024

jennybc commented Nov 9, 2024

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

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

Conversation

jennybc commented Oct 31, 2024 • edited Loading

gaborcsardi commented Oct 31, 2024

jennybc Nov 8, 2024

Choose a reason for hiding this comment

jennybc Nov 8, 2024

Choose a reason for hiding this comment

gaborcsardi Nov 8, 2024

Choose a reason for hiding this comment

jennybc Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

jennybc Nov 8, 2024

Choose a reason for hiding this comment

jennybc commented Nov 8, 2024

gaborcsardi commented Nov 8, 2024

gaborcsardi left a comment

Choose a reason for hiding this comment

jennybc commented Nov 8, 2024 • edited Loading

jennybc commented Nov 8, 2024

gaborcsardi commented Nov 8, 2024

jennybc commented Nov 9, 2024

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

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

jennybc commented Oct 31, 2024 •

edited

Loading

jennybc Nov 8, 2024 •

edited

Loading

jennybc commented Nov 8, 2024 •

edited

Loading