-
Notifications
You must be signed in to change notification settings - Fork 154
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
Conversation
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 |
There was a problem hiding this 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) { |
There was a problem hiding this comment.
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:
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
?
There was a problem hiding this comment.
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.
#Conflicts: # tests/testthat/test-bootstrap.R
@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: