Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error message when using orderly interactively. #120

Merged
merged 8 commits into from
Jan 23, 2024
Merged

Conversation

plietar
Copy link
Member

@plietar plietar commented Jan 18, 2024

It is quite easy and common for users to open up a file in RStudio and start running commands from it interactively, using Ctrl-Enter, without setting their working directory to match that file. When that happens, orderly would throw an unhelpful error, saying only Failed to detect orderly path: /path/to/workspace without any mention of working directories, or that the path should be a report directory (as opposed to eg. the top-level orderly workspace).

The error message is changed to look along the lines of:

! Working directory /path/to/workspace is not a valid orderly report.

This should be more helpful, by making it clear what is incorrect (the working directory), and what was expected (an orderly report).

Additionally, if running in RStudio and the current active editor is a file in what looks like an orderly report, it will augment the error message with a suggestion of what setwd command to run:

! Working directory /path/to/workspace is not a valid orderly report.
ℹ Use `setwd("/path/to/workspace/src/my-report")` to set the working directory
  to the report currently open in RStudio.

Finally, even if the working directory is a valid report, but it does not match the directory of the currently active editor (eg. the user has switched from one report to another), orderly commands will succeed as before, but will show a warning to the user:

Working directory /path/to/workspace/src/my-report does not match the report
currently open in RStudio.
ℹ Use `setwd("/path/to/workspace/src/other-report")` to switch working
  directories.

Frustratingly, while RStudio supports "click-to-run" snippets of code in its output, it restricts the syntax of these and in particular does not allow methods from the base package, such as setwd. Instead the user has to copy-paste the suggestion into the command prompt themselves. See rstudio/rstudio#11273 and rstudio/rstudio#11434. The restriction is pretty arbitrary and we could easily workaround it by re-exporting the setwd function in the orderly2 package if we really wanted to.

@plietar plietar requested a review from richfitz January 18, 2024 12:12
@plietar
Copy link
Member Author

plietar commented Jan 18, 2024

There’s a CI failure that looks unrelated, caused I think by a new version of the cli package that changed the output slightly:
r-lib/cli#569

Will look at fixing that

Copy link
Member

@richfitz richfitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is working well for me.

I don't mind if you fix that cli issue here or in another (or I can do it)

R/interactive.R Show resolved Hide resolved
R/util.R Outdated Show resolved Hide resolved
tests/testthat/helper-orderly.R Outdated Show resolved Hide resolved
DESCRIPTION Show resolved Hide resolved
R/interactive.R Outdated
msg <- c("Working directory {.path {path}} is not a valid orderly report.")
if (!is.null(suggested_wd)) {
cli::cli_abort(
c(msg, i = "Use {.code setwd({.str {suggested_wd}})} to set the working directory to the report currently open in RStudio."))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to use paste() to fix lint about the line length.

We've not been consistent elsewhere about {.code ...} etc and usually just single quote:

"Use 'setwd("{suggested_wd}")' to set the working directory to the report currently open in RStudio."

I was thinking at some point we'd go through the whole lot. I'm not convinced that showing backticks to the user might not be confusing but at the same time I don't feel strongly.

It is quite easy and common for users to open up a file in RStudio and
start running commands from it interactively, using Ctrl-Enter, without
setting their working directory to match that file. When that happens,
orderly would throw an unhelpful error, saying only `Failed to detect
orderly path: /path/to/workspace` without any mention of working
directories, or that the path should be a report directory (as opposed
to eg. the top-level orderly workspace).

The error message is changed to look along the lines of:
```
! Working directory /path/to/workspace is not a valid orderly report.
```

This should be more helpful, by making it clear what is incorrect (the
working directory), and what was expected (an orderly report).

Additionally, if running in RStudio and the current active editor is a
file in what looks like an orderly report, it will augment the error
message with a suggestion of what `setwd` command to run:

```
! Working directory /path/to/workspace is not a valid orderly report.
ℹ Use `setwd("/path/to/workspace/src/my-report")` to set the working directory
  to the report currently open in RStudio.
```

Finally, even if the working directory is a valid report, but it does
not match the directory of the currently active editor (eg. the user has
switched from one report to another), orderly commands will succeed as
before, but will show a warning to the user:

```
Working directory /path/to/workspace/src/my-report does not match the report
currently open in RStudio.
ℹ Use `setwd("/path/to/workspace/src/other-report")` to switch working
  directories.
```

Frustratingly, while RStudio supports "click-to-run" snippets of code in
its output, it restricts the syntax of these and in particular does not
allow methods from the base package, such as `setwd`. Instead the user
has to copy-paste the suggestion into the command prompt themselves.
See rstudio/rstudio#11273 and
rstudio/rstudio#11434. The restriction is pretty
arbitrary and we could easily workaround it by re-exporting the `setwd`
function in the orderly2 package if we really wanted to.
Copy link

codecov bot commented Jan 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d70a1dc) 100.00% compared to head (b6080ef) 100.00%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #120   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           40        40           
  Lines         3564      3589   +25     
=========================================
+ Hits          3564      3589   +25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@richfitz richfitz merged commit ebab7cc into main Jan 23, 2024
11 checks passed
@plietar plietar deleted the mrc-4956 branch January 23, 2024 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants