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

Add check_pre_release_state() #40

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Add check_pre_release_state() #40

wants to merge 19 commits into from

Conversation

pat-s
Copy link
Contributor

@pat-s pat-s commented Jul 17, 2020

State management for pre_release() for #27.

Principle: Skip function execution and print why it happened. Users can still force execution with force = TRUE.

@pat-s pat-s requested a review from krlmlr July 17, 2020 05:35
Copy link
Contributor

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

I wonder if we should have one state function instead that returns the current state as a string, perhaps with verbosity option.

R/state.R Outdated
ui_oops("pre_release(): CRAN-RELEASE already exists.")
state = FALSE
return(state)
} else if (git2r::is_branch(sprintf("cran-%s", update_version_helper(which)))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

update_version_helper() has side effects, I wouldn't expect this from a checker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because desc is modified and avail in the GE? Isn't it recreated on every run?

We don't call $write().

Or is it more about naming and we should rename to update_version_helper_core() or similar?

R/state.R Outdated

# check version number
if (length(strsplit(as.character(desc::desc_get_version()), "[.]|-")[[1]]) < 4) {
ui_oops("pre_release(): Package versions needs to indicate a dev version before calling pre_release().")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use ui_code() inside ui_oops() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. What about #38 ?

R/auto.R Outdated
check_only_modified(character())
pre_release <- function(which = "patch", force = FALSE) {

state = check_pre_release_state(which = which, force = force)
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer <- in this repo.

@pat-s
Copy link
Contributor Author

pat-s commented Jul 17, 2020

I wonder if we should have one state function instead that returns the current state as a string, perhaps with verbosity option.

New issue / PR?

@pat-s
Copy link
Contributor Author

pat-s commented Jul 26, 2020

@krlmlr I've rebased your changes in f-27-automate. How do we want to proceed here? Do you want to follow a different approach?


new_version <- desc$get_version()

return(new_version)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this function return the modified desc object, and be used in update_version_impl() ?

}

# https://github.com/r-lib/desc/issues/93
suppressMessages(desc$bump_version(which))
Copy link
Contributor

Choose a reason for hiding this comment

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

We could also allow the user to specify a version in which, in this case we would call desc$set_version(which) .

Maybe in a new PR?

@krlmlr
Copy link
Contributor

krlmlr commented Aug 24, 2020

This now has conflicts too.

Copy link
Contributor

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Low priority for now.

R/state.R Outdated
@@ -0,0 +1,29 @@
check_pre_release_state <- function(which, force = FALSE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do one function that returns the current state:

  • pre-release (the only state with 4 version number components)
  • releasing, not ready
  • ready to release
  • submitted, waiting
  • accepted

check_release_state(): escape errors if package is not yet on CRAN
@krlmlr
Copy link
Contributor

krlmlr commented Apr 18, 2021

The merge conflicts look trivial, it might be worthwhile resolving them. Still, I'd rather do RC tagging first.

Base automatically changed from f-27-automate to main February 22, 2022 10:13
@maelle
Copy link
Member

maelle commented Oct 16, 2023

@krlmlr what's the status of this, now that there's #686

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants