feat: add which "pre-minor" and "pre-major" to update_version()

[maelle]

Fix #413

Part of #401

As you will see there are two questions in the comments

  # first check there is a fourth component, if not
  # the which should have been another one.
  # at least that's what I understand?

and

# does this assume patch/minor can't already be higher than 99?

[maelle]

I added tests so you can see whether what's here is what you expected.

[krlmlr]

Should we forbid > 99 here? We can roll our own bumping, we won't need suppressMessages() above too.

Perhaps:

new_version <- switch(which,
  "dev" = c(major, minor, patch, (dev + 1) %||% 9000),
  "patch" = {
    if (patch >= 99) abort(...)
    c(...)
  },
  "minor" = ...
)

with explicit local variables for the four components?

[maelle]

Should we forbid > 99 here

And abort if patch/minor are already higher? I had wondered about this in a comment too.

We can roll our own bumping, we won't need suppressMessages() above too.

• Actually the desc issue was solved so that's not needed anyway.
• I find it easier to separate both cases as we need to check the fourth component (dev) is present for "pre-minor" and "pre-major" (at least I assume so: one can't go from not dev to "pre-minor"/"pre-major"?)

[krlmlr]

We want to abort if patch >= 99, as in the code example, overall it looks like it will be simpler without desc::bump_version() .

[maelle]

I added the error. Does the behavior in the test file look ok?

Why would removing

  if (which %in% c("patch", "minor", "major", "dev")) {
    desc$bump_version(which)
    return(desc)
  }

be simpler?

[maelle]

I feel the current code gets the "easiest cases" out of the way right away but I can amend further obviously.

[krlmlr]

What happens if we bump pre-major and then pre-minor?

[krlmlr]

We should do this ourselves, because we want to error:

• in "dev", when dev >= 9999
• in "patch", when patch >= 99
• in "minor", when minor >= 99

[maelle]

I found it easier to follow to use switch once per component (and to still separate the "usual" cases from pre-minor and pre-major).

[krlmlr]

Thanks. I just realized we might want another option: "next", moving from pre-minor to minor and from pre-major to major, and to patch otherwise. Would be the default option to pre_release() . We can do it here or open a new issue.

Do we need more tests for error behavior, e.g. pre-minor after pre-minor, pre-minor after pre-major, and other exotic cases?

[maelle]

I'd also add more tests when tackling the "next" issue.

[maelle]

And at some points the docs will need a diagram.

[krlmlr]

Thanks! Expert nitpicking here, no clear preference.

[krlmlr]

Suggested change

	dev = dev
	)
	new_version <- paste(version_components[!is.na(version_components)], collapse = ".")
	dev = if (!is.na(dev)) dev
	)
	new_version <- paste(version_components, collapse = ".")