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

Closed
wants to merge 19 commits into from
Closed

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 pat-s force-pushed the prerelease-state branch from cdbf36f to d983b09 Compare July 26, 2020 11:50
@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

@pat-s pat-s force-pushed the prerelease-state branch from 2f4a41f to 3b5373c Compare August 24, 2020 15:02
@pat-s pat-s force-pushed the prerelease-state branch from 3b5373c to 5662a5b Compare August 24, 2020 15:04
@pat-s pat-s force-pushed the f-27-automate branch 2 times, most recently from 8f024e2 to e01ee18 Compare August 28, 2020 12:25
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

@krlmlr krlmlr closed this Nov 16, 2024
Copy link
Contributor

aviator-app bot commented Nov 16, 2024

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was closed without merging. If you still want to merge this PR, re-open it.


See the real-time status of this PR on the Aviator webapp.
Use the Aviator Chrome Extension to see the status of your PR within GitHub.

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