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

use_version function #223

Merged
merged 7 commits into from Feb 20, 2018

Conversation

Projects
None yet
3 participants
@EmilHvitfeldt
Copy link
Contributor

EmilHvitfeldt commented Jan 21, 2018

Fixes #188

I would like to resolve #188 and I think I have a solution ready here.

It defaults to an interactive menu where the current version is shown and you are asked which level and how much you would like to increment with. Furthermore does it have arguments for faster non-interactive use. :)

@jennybc

This comment has been minimized.

Copy link
Member

jennybc commented Feb 15, 2018

@hadley Will you indicate your opinion at a high level? If you basically support the goal, I can shepherd the PR further along.

@jennybc jennybc requested a review from hadley Feb 15, 2018

@hadley

This comment has been minimized.

Copy link
Member

hadley commented Feb 15, 2018

Seems like a little too much interactivity right now - I think if you want to increment in some non-standard way, you'll need to use the arguments. Also need to adjust use_dev_version() to use the same underlying code.

Overall seems like a good idea, although the implementation needs some work.

@jennybc

This comment has been minimized.

Copy link
Member

jennybc commented Feb 15, 2018

@EmilHvitfeldt Let me know if you want more concrete advice before proceeding. Otherwise, just ping me when things have advanced.

I came across this function today, which you should probably look into: desc::desc_bump_version(). usethis already depends on desc, so use it freely.

@EmilHvitfeldt

This comment has been minimized.

Copy link
Contributor Author

EmilHvitfeldt commented Feb 16, 2018

Thanks @jennybc! Yes I would like some more concrete advice.

So right now I feel the direction is:

  • A function use_version with 1 layer of interactivity with Mayor, Minor, Patch or Development. With arguments available for nonstandard increment and interactivity-avoidance.
  • Adjustment to use_dev_version(), possible wrapper around use_version with level = Development.

I will look at desc::desc_bump_version() Again. I remember looking at it earlier and deciding against using it.

What convention are using with regard to bumping of .9000. What increment levels should remove .9000 or should it be a separate action?

@jennybc

This comment has been minimized.

Copy link
Member

jennybc commented Feb 17, 2018

A few thoughts:

  • I would hard-wire an increment of 1. That eliminates one interactive query. We just want to cover the most common development tasks.
  • Consider naming the argument bump, i.e. use_version(bump = "minor").
  • Valid values for "bump" would be c("dev", "patch", "minor", "major").
  • I'd present choices approximately like so
Current version is 1.2.0.9000
What part to increment?

1: Major --> 2.0.0
2: Minor --> 1.3.0
3: Patch --> 1.2.1
4: Dev --> 1.2.0.9001

or

Current version is 1.0.0
What part to increment?

1: Major --> 2.0.0
2: Minor --> 1.1.0
3: Patch --> 1.0.1
4: Dev --> 1.0.0.9000

What convention are using with regard to bumping of .9000. What increment levels should remove .9000 or should it be a separate action?

It seems like, if you have a dev version number, bumping the patch version should take you out of a dev version number.

Does this help? At this point, you've probably thought about this more than I have.

Added: I just read the docs on desc_bump_version(). Maybe you should use "which" instead of "bump" to be consistent. It feels like use_version() should be a wrapper around that function. This is appealing from a maintenance point of view.

EmilHvitfeldt added some commits Feb 17, 2018

@EmilHvitfeldt

This comment has been minimized.

Copy link
Contributor Author

EmilHvitfeldt commented Feb 17, 2018

Okay @jennybc, I think this is it. I have

  • hardcoded the increment to 1.
  • removed the second layer of interactivity.
  • presented the resulting version numbers in the first layer of interactivity on the following format

Current version is 2.0.0
what part to increment?

1: major --> 3.0.0
2: minor --> 2.1.0
3: patch --> 2.0.1
4: dev --> 2.0.0.9000

with lowercase words to match the arguments to lessen confusing.

  • changed the argument name to "which".

I tried using the desc_bump_version() function but I could not avoid printing method while keeping the version update. To alleviate that I included a function bump_version in the helpers.R that did the same underlying task.

DESCRIPTION Outdated
@@ -1,6 +1,6 @@
Package: usethis
Title: Automate Package and Project Setup
Version: 1.2.0.9000
Version: 1.1.1.9000

This comment has been minimized.

@jennybc

jennybc Feb 19, 2018

Member

Please remove this change.

@jennybc

This comment has been minimized.

Copy link
Member

jennybc commented Feb 19, 2018

I tried using the desc_bump_version() function but I could not avoid printing method while keeping the version update

I don't quite understand this and would like to. Do you mean it's impossible to just get a version string back from the desc package vs. actually changing the version? I think I'm beginning to see the problem....

R/helpers.R Outdated
@@ -237,3 +237,28 @@ view_url <- function(..., open = interactive()) {
}
invisible(url)
}

bump_version <- function(ver, choice) {

This comment has been minimized.

@jennybc

jennybc Feb 19, 2018

Member

I'd stick with same vocabulary and input values. So "which" instead of "choice" and c("major", "minor", "patch", "dev") as possible values.

@@ -0,0 +1,97 @@
context("use_version.R")

test_that("use_version() increments Mayor by 1 and resets other fields", {

This comment has been minimized.

@jennybc

jennybc Feb 19, 2018

Member

"major"

)
})

test_that("use_version() increments patch by 1 leaving other fields alone", {

This comment has been minimized.

@jennybc

jennybc Feb 19, 2018

Member

Is this a duplicate of the test above?

@jennybc

This comment has been minimized.

Copy link
Member

jennybc commented Feb 19, 2018

I'd shift the all the case-by-case testing to be testing the bump_version() helper. Then it can be much more succinct, i.e. multiple expectations inside one test, and won't need a skip. Then you really only need one test of use_version() to make sure it's wired up correctly.

R/version.R Outdated
bump_version(ver, 3),
bump_version(ver, 4))

if(is.null(which)) {

This comment has been minimized.

@jennybc

jennybc Feb 19, 2018

Member

Feels like there should be an early check for which = NULL in a non interactive session that exits early and does nothing.

@EmilHvitfeldt

This comment has been minimized.

Copy link
Contributor Author

EmilHvitfeldt commented Feb 19, 2018

I don't quite understand this and would like to. Do you mean it's impossible to just get a version string back from the desc package vs. actually changing the version? I think I'm beginning to see the problem....

Yes the desc package will calculate the updated version string and update the DESCRIPTION at the same time.

@jennybc

This comment has been minimized.

Copy link
Member

jennybc commented Feb 19, 2018

Thanks to @gaborcsardi:

There is a way to use desc's version incrementation logic w/o changing the version of the active package:

r-lib/desc#61 (comment)

Are you game to put this inside bump_version()? I would like to NOT maintain a separate version of this logic but, instead, get it from desc.

@EmilHvitfeldt

This comment has been minimized.

Copy link
Contributor Author

EmilHvitfeldt commented Feb 19, 2018

That is amazing! I put it inside bump_version().

@jennybc

This comment has been minimized.

Copy link
Member

jennybc commented Feb 19, 2018

Thanks! Will you add a bullet to the dev version section of NEWS? I might groom this a bit after the merge but will cc you on those commits, so you're in the loop.

@EmilHvitfeldt

This comment has been minimized.

Copy link
Contributor Author

EmilHvitfeldt commented Feb 20, 2018

I'm a little unsure of how I would change the NEWS.md file since my forked directory is behind and doesn't have the up to date NEWS.md.

@jennybc

This comment has been minimized.

Copy link
Member

jennybc commented Feb 20, 2018

I'm a little unsure of how I would change the NEWS.md file since my forked directory is behind and doesn't have the up to date NEWS.md.

OK I'll take it from here then.

Next time, you should create a branch in your fork, then do your pull request work in that branch. If master changes during the process, you would pull those changes into master in your fork, then merge master into your branch.

But this PR is fine. We are developing a guide for this, but it's not in position yet.

Thanks!

@jennybc jennybc merged commit 617e24f into r-lib:master Feb 20, 2018

4 checks passed

codecov/patch 75.6% of diff hit (target 61.95%)
Details
codecov/project 62.03% (+0.08%) compared to dd1b390
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.