Fixes and improvements for strategies #26
+294
−46
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,7 +96,19 @@ get_ref_min.remote_ref_cran <- function(remote_ref, op = "", op_ver = "") { | |
| min_ver <- Filter(function(x) x == min(pv), pv) | ||
|
|
||
| new_ref <- sprintf("%s@%s", remote_ref$ref, names(min_ver)) # @TODO deparse, add ver, parse again | ||
| tryCatch( | ||
|
There was a problem hiding this comment. I am just curious - can we move this try to the generic so that other types of remote will be covered without the need of copy&paste same error message |
||
| pkgdepends::parse_pkg_ref(new_ref), | ||
| error = function(err) { | ||
| cli::cli_alert_danger( | ||
| paste( | ||
| sep = " ", | ||
| "Possible problem finding release for:", | ||
| "`{remote_ref$package} ({op} {op_ver})`.", | ||
| "The version might be invalid." | ||
| ) | ||
| ) | ||
| stop(err) | ||
| }) | ||
| } | ||
|
|
||
| #' @rdname get_ref_min | ||
|
|
@@ -150,7 +162,13 @@ get_ref_min.remote_ref_github <- function(remote_ref, op = "", op_ver = "") { | |
| } | ||
| } | ||
|
|
||
| new_ref <- sprintf( | ||
| "%s=%s/%s%s", | ||
| remote_ref$package, | ||
| remote_ref$username, | ||
| remote_ref$repo, | ||
| ref_suffix | ||
| ) | ||
| pkgdepends::parse_pkg_ref(new_ref) | ||
| } | ||
|
|
||
|
|
@@ -163,6 +181,7 @@ get_gh_refs <- function(org, repo) { | |
| } | ||
| get_gh_tags(org, repo) | ||
| } | ||
|
|
||
| #' @importFrom gh gh_gql | ||
| #' @keywords internal | ||
| get_gh_releases <- function(org, repo, max_date = Sys.Date() + 1, min_date = as.Date("1900-01-01")) { | ||
|
|
@@ -187,6 +206,7 @@ get_gh_releases <- function(org, repo, max_date = Sys.Date() + 1, min_date = as. | |
| ) | ||
| vapply(res, `[[`, character(1), "tagName") | ||
| } | ||
|
|
||
| #' @importFrom gh gh_gql | ||
| #' @keywords internal | ||
| get_gh_tags <- function(org, repo, max_date = Sys.Date() + 1, min_date = as.Date("1900-01-01")) { | ||
|
|
@@ -316,40 +336,76 @@ cond_parse_pkg_ref_release <- function(remote_ref) { | |
| get_release_date <- function(remote_ref) { | ||
| UseMethod("get_release_date", remote_ref) | ||
| } | ||
|
|
||
| #' Get release date from GitHub references | ||
| #' | ||
| #' @inheritParams get_release_date | ||
| #' | ||
| #' @importFrom gh gh_gql | ||
| #' @export | ||
| #' @examplesIf gh::gh_token() != "" | ||
| #' remote_ref <- pkgdepends::parse_pkg_ref("insightsengineering/[email protected]") | ||
| #' get_release_date.remote_ref_github(remote_ref) | ||
| get_release_date.remote_ref_github <- function(remote_ref) { | ||
| gql_query <- sprintf("{ | ||
| repository(owner: \"%s\", name: \"%s\") { | ||
| refs(refPrefix: \"refs/tags/\", query: \"%s\", first: 100) { | ||
| edges { | ||
| node { | ||
| name | ||
| target { | ||
| ... on Commit { | ||
| committedDate | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }", remote_ref$username, remote_ref$repo, remote_ref$commitish) | ||
|
|
||
| resp <- try(gh::gh_gql(gql_query), silent = TRUE) | ||
| if (inherits(resp, "try-error")) { | ||
| return(character(0)) | ||
| } | ||
|
|
||
| result <- vapply( | ||
| resp$data$repository$refs$edges, | ||
| function(x) { | ||
| if (x$node$name != remote_ref$commitish) return(NA_character_) | ||
| x$node$target$committedDate | ||
| }, | ||
| character(1) | ||
| ) | ||
|
|
||
| if (length(result) <= 1) { | ||
| return(result %||% NA_character_) | ||
| } | ||
|
|
||
| max(result, na.rm = TRUE) | ||
| } | ||
|
|
||
| #' Get release date from GitHub references | ||
| #' | ||
| #' @inheritParams get_release_date | ||
| #' | ||
| #' @export | ||
| #' @examplesIf Sys.getenv("R_USER_CACHE_DIR", "") != "" | ||
| #' remote_ref <- pkgdepends::parse_pkg_ref("[email protected]") | ||
| #' get_release_date.remote_ref_cran(remote_ref) | ||
| get_release_date.remote_ref_cran <- function(remote_ref) { | ||
| subset( | ||
| get_cran_data(remote_ref$package), | ||
| package_version(version, strict = FALSE) == package_version(remote_ref$version, strict = FALSE), | ||
| mtime | ||
| )[[1]][1] | ||
| } | ||
|
|
||
| #' @export | ||
| get_release_date.remote_ref_standard <- function(remote_ref) { | ||
| get_release_date.remote_ref_cran(remote_ref) | ||
| } | ||
|
|
||
| #' @export | ||
| get_release_date.remote_ref <- function(remote_ref) { | ||
| NA | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 still struggle to fully understand what it does and why it is there.
You have the following:
d$get_remotes()[!(remotes_pkg %in% new_ref_pkg)]is a vector of remotes not present in config/needs/verdepchecknew_ref_remoteis vector with config/needs/verdepcheck refs for packages existing in remotes fieldSo essentially it's (conditionally) replacing remotes entries with config/needs/verdepcheck entries. OK seems that I get what it does while writing this comment.
UPDATE: I just came up with example where you actually remove some entries so it's not only about replacing one with another. Then I guess we can modify for loop with
if (cond1) then NULL else if (cond2) x else y$get_remotes()twice - also not a big dealnew_refs,new_ref_remote,new_ref_remoteand none of them indicates new references for remote fieldHow about a simple for loop:
desc_cond_set_refsfirst and thendesc_remotes_cleanupusing one argument only indicating desc object. This could potentially simplify naming issue as well as it fits to the role better - you want to replace desc remotes field with another field (that is being extracted from the same desc object and not another argument)Please have a look at
get_refs_from_descexecuted at the very beginning of each installation proposal constructor - it is that function which is responsible for preparing correct list of references for further analysis. I would even ask if we need a Remotes at all - as we already / should have everything we need in the config/needs/verdepcheck. That field can potentially have downstream implications so I am inclined to request to always clear it.