-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat: Improve bump_version() behavior in the absence of changes #323
Conversation
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.
Thanks, I like the tests a lot. Let's do another pass.
R/bump-version.R
Outdated
#' @description | ||
#' 1. Verify that the current branch is the main branch. | ||
check_main_branch() | ||
#' 1. Check there were changes since the last version.no_change() |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
R/api-bump-version.R
Outdated
check_which(which) | ||
|
||
no_change_behavior <- match.arg(no_change_behavior, choices = c("bump", "noop", "fail")) |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
R/bump-version.R
Outdated
@@ -78,3 +93,7 @@ get_main_branch_config <- function() { | |||
global <- init[init$level == "global"] | |||
return(global$value) | |||
} | |||
|
|||
no_change <- function() { | |||
nrow(default_commit_range()) == 1 |
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.
Why 1? Can you please add a comment? Should we do <= 1
for safety?
@@ -22,7 +22,7 @@ collect_news <- function(messages) { | |||
unlist() | |||
|
|||
if (length(message_items) == 0) { | |||
if (length(range) <= 1) { | |||
if (length(messages) <= 1) { |
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.
The logic seems repeated here. Should no_change()
get a messages
argument?
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've changed the variable names to clarify that we are looking for newsworthy_items
among messages
.
Even if no_change()
was run before getting to this point, if the argument no_change_behavior
could mean we arrive at this point.
But maybe in the case there's less than or 1 message the function should exit earlier, is this what you mean?
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.
Should we have no_change()
handle the <= 1
comparison, so that this logic is centralized, if it makes sense?
@@ -22,7 +22,7 @@ collect_news <- function(messages) { | |||
unlist() | |||
|
|||
if (length(message_items) == 0) { | |||
if (length(range) <= 1) { | |||
if (length(messages) <= 1) { |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Snapshot updates for rcc-smoke (null)
Co-authored-by: Kirill Müller <krlmlr@users.noreply.github.com>
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.
Thanks, looks great!
Fix #316
Fix #307