-
Notifications
You must be signed in to change notification settings - Fork 69
Allow build numbers to be arbitrary strings or null #90
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
base: master
Are you sure you want to change the base?
Allow build numbers to be arbitrary strings or null #90
Conversation
|
@danielflower I have more new features coming that are based in this branch. Hence, I would appreciated if you could provide guiding feedback soon, thanks. |
|
@danielflower Besides this and #94 I have more improvements in mind for this plugin. Yet, I must say not even getting any feedback on PRs is very de-motivating. What are your plans with this project if I may ask? |
|
Hi Marcel, sorry I know it's frustrating to not get feedback. I just really don't have much time to go through things very often. Merging a pull request is a commitment on my behalf as I have to understand what is happening on code I have touched for years and then support those changes going forward, so I need to find enough time to go through changes. (The smaller the change, the easier to do.) As for this change, this feels like quite a bit of a change from the current logic of "the build number gets incremented". I don't really understand what happens when it tries to increment a string. |
|
I'll be happy to answer any questions - just keep'em coming.
Most of changes are caused by the long-to-String switch for the build number (many method signatures, lots of tests). And then there are the new tests of course.
To me the only really interesting corner case is if you switch from numeric build numbers (past) to alpha-numeric (now and possibly for the future) when the business version remains as-is.
Or |
src/main/java/com/github/danielflower/mavenplugins/release/Reactor.java
Outdated
Show resolved
Hide resolved
This required changing the `buildNumber` data type from `long` to `String`. Fixes danielflower#75
1c661aa to
702119d
Compare
|
All review comments addressed from my side. |
|
Can I do anything to help land this on master? |
|
The reason I've not merged this so far is that in my mind this is a fundamental change to the plugin. In my mind, the plugin generates build numbers for you by adding 1 to the previous number. I'm probably missing something though. Maybe some examples of build numbers in the pom.xml vs build numbers in a release? e.g. currently if the pom version is |
Don't worry, it's not 😄 The original behavior does not change at all. Unless you (as a user) explicitly want so the plugin behavior remains as-is. There are situations, though, where this strict build number increment is undesirable. You could want your build number to be "A" or "foo" - or nothing at all. The Javadoc for Hence, to allow such flexibility I had to change the data type from |
|
Hi, I don't know if you are still interested in this, so long afterwards, but if you want to get it merged I think all it's missing is an explanation of when this can be used, why it would be used, and what happens to build numbers after using it (this lack of understanding on my behalf is why I took so long to take the time to understand this, so it's probably confusing to others too). I think a section on https://danielflower.github.io/multi-module-maven-release-plugin/usage.html would be good. Something like this?
Perhaps work in the clear explanation that @cdprete originally wrote on #75
(Does the above logic hold for this PR?) The documentation needs some concrete examples, e.g. if I have I must say though, as a user of |
|
If I got your last comment right then I guess the "lack of understanding" on your behalf is a (maybe) simple misunderstanding. This change here only affects the build number i.e. anything you specify after the - ideally semantic - version. This is limited to the part that the plugin modifies by automatically incrementing. So, for your example it wouldn't be |
|
Yeah, I guess I never understood this PR then. So if I have So this is more of a suffix to the version than a build number? Then why not leave it as a |
This required changing the
buildNumberdata type fromlongtoString. @cdprete this fixes your issue #75. Hence, your feedback is appreciated.