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

feat: Pull as part of finalize_version() #408

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

Conversation

maelle
Copy link
Member

@maelle maelle commented Jul 26, 2022

Fix #39

Not ready, I need some clarifications. (maybe a concrete example of a stage where pull=TRUE makes sense, as well as a concrete example of a stage where pull=FALSE makes sense).

@maelle maelle mentioned this pull request Jul 26, 2022
2 tasks
#' @example man/examples/commit-version.R
#'
#' @export
commit_version <- function() {
amending <- commit_version_impl()
commit_version <- function(pull = TRUE) {
Copy link
Member Author

Choose a reason for hiding this comment

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

actually the default should be FALSE I suppose

@maelle maelle requested a review from krlmlr August 29, 2022 09:26
@krlmlr
Copy link
Contributor

krlmlr commented Aug 30, 2022

Thanks. pull = FALSE is required if offline. I'm not sure we need it for the lower-level functions such as commit_version() .

@maelle
Copy link
Member Author

maelle commented Sep 1, 2022

I'm not sure we need it for the lower-level functions such as commit_version() .

So you'd prefer the code

  if (has_remote_branch(gert::git_branch()) && pull) {
    # is the local branch behind?
    if (gert::git_ahead_behind()$behind > 0) {
      # With pull = TRUE we would fetch always and always uncommit + commit if behind master
      gert::git_pull(rebase = TRUE)
    }
  }

to be directly in finalize_version()?

Another question: is the default pull = TRUE ok?

@krlmlr
Copy link
Contributor

krlmlr commented Sep 5, 2022

I'd wrap that code in a function and call it only from finalize_version(), perhaps.

Rethinking offline work. If we're offline, or if PATs are unset, everything should still work, but require opt-in. Otherwise, the default should be to require connectivity and PATs, and do everything that's needed to keep the work in sync. Perhaps we need bump_version(allow_offline = FALSE) ?

@maelle
Copy link
Member Author

maelle commented Sep 5, 2022

Rethinking offline work. If we're offline, or if PATs are unset, everything should still work, but require opt-in. Otherwise, the default should be to require connectivity and PATs, and do everything that's needed to keep the work in sync. Perhaps we need bump_version(allow_offline = FALSE) ?

Should this be a separate issue?

  • asking users to confirm they want to use the function offline.
  • allowing an easy "refresh" once they are online again (it'd probably be easier to unbump then rebump, actually).

One way to lose less information when offline is

But offline, one would still not have the info on PR attribution. Maybe for that too it'd be good to ensure the info is stored in the commit messages via "Co-Authored-by".

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.

Thanks. Now that everything's on GHA, this becomes much lower priority.

#' @description
#' 1. [commit_version()]
force <- commit_version()
#' 1. [tag_version()], setting `force = TRUE` if and only if `commit_version()`
#' amended a commit.
tag <- tag_version(force)
#' 1. git pull with rebase if `pull = TRUE`.
if (has_remote_branch(gert::git_branch()) && pull) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should pull = TRUE fail if there's a remote branch?

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.

Pull as part of finalize_version()
2 participants