-
Notifications
You must be signed in to change notification settings - Fork 2
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
Staging fixes #56
Staging fixes #56
Conversation
…rt of the freeze.
Also adds |
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.
Looks good! Just some minor comments.
X = seq_len(nrow(meta)), | ||
FUN = function(index) { | ||
utils::compareVersion( | ||
a = meta$version[index], |
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.
Just FYI this is something I subscribe to: https://style.tidyverse.org/syntax.html#function-calls omitting the names of (compulsory) data arguments.
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.
I'll keep that in mind for the future.
json$url <- json[["_failure"]]$buildurl %||% rep(NA_character_, nrow(json)) | ||
success_source <- is.na(json$url) |
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.
Seems a roundabout way of reading successes?
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.
On source failure, the correct URL (with the error logs) comes from _failure$buildurl
and not the top-level _buildurl
. These few lines just say to start with _failure$buildurl
and then fill in with _buildurl
when needed. The success_source
variable just makes it clearer what exactly is.na(json$url)
is trying to achieve.
}, | ||
FUN.VALUE = logical(1L) | ||
) | ||
meta[[repo]][!conflict] <- NA_character_ |
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.
Is there any advantage to using NA_character_
vs. NA
(globally)?
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.
I like NA_character_
for the sake of defensive programming. NA
is actually a logical type, but the element is supposed to be a character. I can never predict when a difference in types will cause a subtle bug.
Co-authored-by: Charlie Gao <[email protected]>
Co-authored-by: Charlie Gao <[email protected]>
propose_snapshot()
so downstream automation can grep the snapshot URL for the version instead of needing to install R and therversions
package.c.f. r-multiverse/help#129