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.

Sign up for GitHub

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

Jump to bottom

Refine auto-loading behaviour #1915

Merged
merged 8 commits into from
Jul 2, 2024
Merged

Refine auto-loading behaviour #1915

Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
beb8d61
Re-order these checks
jennybc May 29, 2024
2fb9e65
Put some DOTS on it
jennybc May 31, 2024
f24aba7
Be precise about the recommended action
jennybc May 31, 2024
9d8635f
Don't message about cancellation
jennybc Jun 4, 2024
1989c79
Refine action around renv::restore() during autoloading
jennybc Jun 7, 2024
a11676f
Make `renv::restore()` (maybe) a runnable hyperlink if cli is available
jennybc Jun 7, 2024
ec7bf57
Introduce `cancel(verbose =)`
jennybc Jun 24, 2024
f23c248
Back out of the cli experiment
jennybc Jun 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions R/load.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ load <- function(project = NULL, quiet = FALSE, profile = NULL, ...) {

action <- renv_load_action(project)
if (action[[1L]] == "cancel") {
cancel()
quietly(cancel())
jennybc marked this conversation as resolved.
Show resolved Hide resolved
} else if (action[[1L]] == "init") {
return(init(project))
} else if (action[[1L]] == "alt") {
Expand Down Expand Up @@ -146,22 +146,22 @@ renv_load_action <- function(project) {
if (!interactive())
return("load")

# if this project already contains an 'renv' folder, assume it's
# already been initialized and we can directly load it
renv <- renv_paths_renv(project = project, profile = FALSE)
if (dir.exists(renv))
return("load")

jennybc marked this conversation as resolved.
Show resolved Hide resolved
# if we're running within RStudio at this point, and we're running
# within the auto-loader, we need to defer execution here so that
# the console is able to properly receive user input and update
# https://github.com/rstudio/renv/issues/1650
autoloading <- getOption("renv.autoloader.running", default = FALSE)
if (autoloading && renv_rstudio_available()) {
setHook("rstudio.sessionInit", function() { renv::load(project) })
setHook("rstudio.sessionInit", function(...) { renv::load(project) })
Copy link
Member Author

@jennybc jennybc May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Before, renv::load(project) was definitely being appended to the "rstudio.sessionInit" hook. I can see that with getHook("rstudio.sessionInit"). However it wasn't actually being executed and I think it must have failed some pre-check re: its signature. In my hands, adding ... makes this feature start to work as intended.

Copy link
Member Author

@jennybc jennybc May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is the explanation:

https://github.com/rstudio/rstudio/blob/8ebe5431ca2fb3c9719b70d4fbc45201d226c79c/src/cpp/session/modules/SessionRHooks.R#L26-L29

RStudio forwards arguments and the whole thing happens inside tryCatch(). So I guess if the hook function does not accept any arguments, the attempt to run the hook fails. Silently.

return("cancel")
jennybc marked this conversation as resolved.
Show resolved Hide resolved
}

# if this project already contains an 'renv' folder, assume it's
# already been initialized and we can directly load it
renv <- renv_paths_renv(project = project, profile = FALSE)
if (dir.exists(renv))
return("load")

# check and see if we're being called within a sub-directory
path <- renv_file_find(dirname(project), function(parent) {
if (file.exists(file.path(parent, "renv")))
Expand Down Expand Up @@ -881,7 +881,7 @@ renv_load_report_synchronized <- function(project = NULL, lockfile = NULL) {
return(FALSE)
}

response <- ask("Would you like to restore the project library?", default = FALSE)
response <- ask("Would you like to run `renv::restore()` to restore the project library?", default = FALSE)
Copy link
Member Author

Choose a reason for hiding this comment

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

It feels like this could ease decision-making for the user, i.e. it's easier to answer this question confidently if you know exactly what's being proposed. Also might have some value in terms of user education.

if (!response)
return(FALSE)

Expand Down
Loading