zafarkhaja / jsemver

Java implementation of the SemVer Specification
MIT License
429 stars 82 forks source link

Support for removing buildMetadata or preReleaseVersion #16

Closed adamdubiel closed 9 years ago

adamdubiel commented 9 years ago

0.8.0 version throws IllegalArgumentException when setting buildMetadata or preReleaseVersion to null or <empty string>. All in all i like the change, but i would also like to have a way to easily create version without them, something like:

Version withoutMetadata = version.clearBuildMetadata();

I could create PR for that if you think it fits your design.

zafarkhaja commented 9 years ago

Hey @adamdubiel, thanks for the feedback.

The reason why I think nulls and <empty string>s shouldn't be allowed here is that to me it feels more like a programming error rather than a valid use case. Consider the following code snippet

Version version = Version.forIntegers(1, 0, 0);
version = version.setBuildMetadata(generateUniqueHash());

What should one expect from this if generateUniqueHash() fails? Well, obviously not this 1.0.0+ because it's not valid according to the SemVer grammar. Most likely this 1.0.0, but still silently ignoring the error is no better because it may lead to unexpected consequences, like assigning the same version tag to two or more different code bases. Therefore I chose to inform the client programmer about the error. In this regard, version.clearBuildMetadata() is a better option because it clearly conveys the intent.

However, the good news is that there's already a Version.Builder class which was designed specially for cases like yours, it's less strict and does allow nulls and <empty string>s. Or, you could do something along these lines

...
if (version.preReleaseVersion == VersionService.SNAPSHOT) {
    String tagName = versionConfig.tag.serialize(versionConfig.tag, version.getNormalVersion())
    ...
}
...

Hope this helps. Let me know if you need any further assistance.

adamdubiel commented 9 years ago

Of course i agree with null and <empty string> approach. I didn't notice there is new Builder there. That should be enough for me, thanks for help :)