-
Notifications
You must be signed in to change notification settings - Fork 163
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: adding prerelease version increments #397
feat: adding prerelease version increments #397
Conversation
(I can take a look next week sometime, but if someone else wants to look first, that would be welcome) |
README.md
Outdated
The default settings make use of the helper class [`Version`](https://github.com/sbt/sbt-release/blob/master/src/main/scala/Version.scala) that ships with *sbt-release*. | ||
|
||
```scala | ||
// strip the qualifier off the input version, eg. 1.2.1-SNAPSHOT -> 1.2.1 | ||
releaseVersion := { ver => Version(ver).map(_.withoutQualifier.string).getOrElse(versionFormatError(ver)) } | ||
|
||
// bump the version and append '-SNAPSHOT', eg. 1.2.1 -> 1.3.0-SNAPSHOT | ||
releaseNextVersion := { | ||
ver => Version(ver).map(_.bump(releaseVersionBump.value).asSnapshot.string).getOrElse(versionFormatError(ver)) | ||
}, | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned on Discord, my guess is that @xuwei-k's primary concern is breaking this contract that's been around for 10+ years in existing builds in the wild. In other words, you should expect that there are builds out there that literally have those customization e.g.:
// strip the qualifier off the input version, eg. 1.2.1-SNAPSHOT -> 1.2.1
releaseVersion := { ver => Version(ver).map(_.withoutQualifier.string).getOrElse(versionFormatError(ver)) }
and the reasonable expectation is that this plugin should continue to function with those customization, until it decides to break the compatibility. So if there's breaking change in the README, it's a red flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick review!
I don't see this as breaking contract - simply making it more honest and user-friendly. This PR does not remove or change the type signatures of any public entities. Nor does it prevent users from overriding these changes with their own values (i.e. the code linked in the README above will still work and produce exactly the same result as before, without a prerelease increment). The only change is that prerelease versions will, by default, be incremented.
From a user perspective, I see sbtrelease
saying that it increments versions by default to the Next
version. I assumed that this meant that the last version number is incremented, including any prerelease version number. But sbtrelease
does not increment prerelease version numbers; it simply removes the prerelease altogether behind the scenes. Call it part of the established contract over 10 years, but (to me, at least, and I suspect to other users as well) it's unintuitive. Not a bug, but the contract is (slightly) lying to you.
Obviously if you're creating a numbered prerelease, your default expectation is that there will be multiple of them, not that the prerelease goes to final after the first one. That's where this PR comes in - a version.sbt
with 1.0.0-RC2-SNAPSHOT
by default sets releaseVersion
to 1.0.0-RC2
, instead of 1.0.0
, and releaseNextVersion
to 1.0.0-RC3-SNAPSHOT
, instead of 1.0.1-SNAPSHOT
. That's it.
Note: I marked .withoutQualifier
as deprecated (but I didn't remove it), because I noticed that in this code base as well as in others it is just being used to remove the snapshot. Hence the new .withoutSnapshot
method and the note encouraging users to migrate. If that's controversial, then I will change it so that the two methods simply live side by side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's what I'd suggest (and and also if similar situation comes up in the future): please open a GitHub issue with steps (1.0.0-RC2-SNAPSHOT
), problems (it doesn't bump to RC3), and expectations (it bumps to RC3). Hopefully the issue would clarify the problem domain, which you rightly feel should be fixed and the existing user probably has not cared since sbt-release has not bumped it all this time. My guess is that current users have been either using SNAPSHOT or full patch versions, without RC cycle.
Although I have no horse in this race, I am generally +1 on supporting this M1/RC1 use case for sbt-release, so long as we can document the breaking/migration aspect clearly in README and release note. Instead of removing the old customization from the README (or deprecating), I suggest we document it like:
RC cycle support
sbt-release x.y.z adds RC cycle support. Customize the keys as follows:
....users who don't need RC cycle
you can keep using the old settings:
....
This way 100% of the existing users who are not using RC cycle can keep their old settings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created an issue here, #398, but I think there's some confusion about my changes to the README. Before my PR, this 8-line section in the README here is just describing the default behavior by literally copying and pasting the current code base here. I've made changes to the README because it shouldn't reproduce the code base. This defeats the purpose of a README - if I want to read the code, of course I don't need a README to do that, but it's also not a maintainable pattern in general, nor is it the most readable.
So what I have removed from the README did not represent a customization (which would still work in the same way if users actually copied and pasted that in), but just the current default behavior.
That said, are we fine with prerelease increments becoming part of the new default behavior? Ironically, if yes and we want to add a note to help users keep the old settings, I will need to re-include the README code snippet, since it will be necessary and won't reproduce the code base anymore.
@@ -2,3 +2,4 @@ target | |||
project/target | |||
.idea | |||
.idea_modules | |||
.bsp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally speaking we are fine with merging .gitignore
changes, but regardless, I'll say: you'll probably want to add .bsp
to your global gitignore, so then you won't need to submit this change to every repository that you contribute to.
I don't have any special insight here. I'll make the very general observation that I think breaking changes to sbt plugins are okay, if they are 1) reasonably well motivated, 2) documented, 3) signaled by bumping the minor version (or major, but I think that would be overkill in this case), 4) highlighted in the release notes. Compatibility's is good, but build-time changes that the compiler will alert users to really aren't much of a burden to put on users. Only silent breakage is pernicious. |
2c06d3c
to
32f9c07
Compare
I have updated this PR to reflect the outcome of the conversation in #398. I also updated the PR description so that there's a handy summary of all changes present. Please let me know if you have any questions or if there's anything more that I can do. |
Background Summary
This PR adds support for incrementing prerelease versions by default, if it ends in a number. Currently, if a prerelease version is incremented, the prerelease qualifier is simply dropped. E.g.
1.0.0-RC1
will be incremented to1.0.0
. After this merge,1.0.0-RC1
will be incremented to1.0.0-RC2
, but prerelease versions without a version number will behave as before:1.0.0-alpha
will be incremented to1.0.0
.New/Updated Versioning Strategies
Next
(updated - breaking change): Will now increment prerelease versions, unlike in the past. So1.0-RC1
will become1.0-RC2
. Previously1.0-RC1
would become1.1
.NextStable
(new): The same asNext
except that it excludes any prerelease versions. So1.0.0-RC1
becomes1.0.0
New/Updated Methods in Version class
.asSnapshot
(updated - breaking change) will now include the prerelease version. E.g.1.0.0-RC1
will return1.0.0-RC1-SNAPSHOT
. Previously,1.0.0-RC1
would become1.0.0-SNAPSHOT
..withoutSnapshot
(new) will remove only the snapshot part of the ending. E.g.1.0.0-RC1-SNAPSHOT
will become1.0.0-RC1
..bumpNext
(new) will bump the version according to theNext
strategy..bumpNextStable
(new) will bump the version according to theNextStable
strategy..bump
(deprecated) will now point to.bumpNext
Miscellaneous Changes
Removed emitted warnings and updated deprecated
processResult
method toprocessResult2
.Deprecated
.string
on the Version class and created a new method.unapply
with the same functionality..string
is too similar to.toString
(just in this simple PR, I created an annoying bug by accidentally typing.toString
when I meant to type.string
) and it doesn't follow the standard apply/unapply pattern. Since the Version case class already has a custom.apply
method with a string input, it ought to have an.unapply
method with a string output.Updated README to reflect that certain keys are now task keys and not setting keys. This is just a documentation change - the underlying functionality was changed some time ago in #194, but the README was never updated.
Feature History
A previous PR to implement this feature was merged and then reverted without comment here: #393.
Changes introduced by the reviewer created a bug in #393. This was fixed in #395, but closed with no relevant comment - all changes there were necessary.
Use case - we are planning to release a number of prerelease versions before an initial rollout and these few changes would make that a lot easier.
Many thanks in advance for a review!