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

Support GitHub sha as version #1327

Merged
merged 18 commits into from
May 25, 2023
Merged

Support GitHub sha as version #1327

merged 18 commits into from
May 25, 2023

Conversation

hadley
Copy link
Member

@hadley hadley commented May 24, 2023

@kevinushey I think this is worth taking a look at now.

I'm not sure how to test it. Interactively, I did this in a new project:

pak::pak("rstudio/renv#1327")
renv::init()

unlink("renv/library", recursive = TRUE)
source("renv/activate.R")

@hadley hadley requested a review from kevinushey May 24, 2023 18:39
@hadley
Copy link
Member Author

hadley commented May 24, 2023

We also need to think through if this is going to cause problems for us, given that I, at least, usually install renv with RStudio, not install_github() so it doesn't have an associated sha.

Copy link
Collaborator

@kevinushey kevinushey left a comment

Choose a reason for hiding this comment

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

Overall LGTM

@@ -827,3 +887,14 @@ renv_bootstrap_user_dir_impl <- function() {

}

renv_bootstrap_version_is_dev <- function(version) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should bring in R/version.R here:

renv/R/zzz.R

Lines 113 to 116 in 8b145e8

# read the necessary bootstrap scripts
scripts <- c("R/bootstrap.R", "R/json-read.R")
contents <- map(scripts, readLines)
bootstrap <- unlist(contents)

and use those utilities here? I'm not sure what we would call this function though; maybe it suffices to just check renv_version_length(version) == 3L to infer that it's a release version of renv?

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 still needs slightly different code because it's slightly different test to figure out if we're using a sha or a version number. So IMO, it's not worth it.

@hadley hadley changed the title WIP: support GitHub sha as version Support GitHub sha as version May 24, 2023
@hadley hadley marked this pull request as ready for review May 24, 2023 19:58
@hadley hadley merged commit e73c689 into main May 25, 2023
@hadley hadley deleted the sha-from-description branch May 25, 2023 12:01
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