feat!: make version comparison semver compliant #115
Conversation
…xclude SNAPSHOT from lower bound
jdrueckert
added
the
Type: Improvement
|
There was a problem hiding this comment.
This abandons the attempt at moving the compatible-version logic to the JSemVer library. But since writing that I have found out that gradle's default version-compatibility logic is not nearly as semver-compatible as I thought it was. VersionRange is a pretty good model as a thing to translate to gradle after all, as gradle doesn't have the ^ range comparison operator.
I marked a nit or two but I am good with the overall approach.
Suggestion: I think this got rid of the things that needed Version.semver to be package-visible:
so we can make that private again and revert 4b6ea6d (should do before release)
| @@ -44,7 +44,7 @@ public Version getUpperBound() { | |||
| * @return Whether version falls within the range | |||
| */ | |||
| public boolean contains(Version version) { | |||
| return version.compareTo(lowerBound.getSnapshot()) >= 0 && version.compareTo(upperBound.getSnapshot()) < 0; | |||
| return version.compareTo(lowerBound) >= 0 && version.compareTo(upperBound.getSnapshot()) < 0; | |||
There was a problem hiding this comment.
This works for Terasology's current use cases. I agree that having one VersionRange class is better than having the one-foot-in-one-foot-out thing I had going on.
The edge cases this falls apart on are things like 1.4.7-SNA or 1.4.7-SNAPSHOT-2. Which—maybe we don't care? Would be fine with declaring that out of scope. But that is the sort of thing that motivated me to try to delegate the problem to the SemVer library (since we have it anyway).
There was a problem hiding this comment.
That sounds like completely removing VersionRange would be another option.
Technically, SemVer does not describe anything special about -SNAPSHOT (I think this is a maven concept). All of the -.... exmaples are pre-releases, and none of them matches a version requirement for the released version.
gestalt-module/src/main/java/org/terasology/gestalt/naming/Version.java
Outdated
Show resolved
Hide resolved
...-module/src/main/java/org/terasology/gestalt/module/dependencyresolution/DependencyInfo.java
Outdated
Show resolved
Hide resolved
| if (minVersion.getMajor() == 0) { | ||
| return minVersion.getNextMinorVersion(); | ||
| return minVersion.getVersionCore().getNextMinorVersion(); | ||
| } | ||
| return minVersion.getNextMajorVersion(); | ||
| return minVersion.getVersionCore().getNextMajorVersion(); |
There was a problem hiding this comment.
thought: this is another one of those places that makes me think "if we have to keep this if "0.x" logic ourselves, what good is having JSemVer?"
There was a problem hiding this comment.
suggestion (non-blocking): regardless of JSemVer, we could move this to a method on Version. "getExclusiveUpperCompatibleRangeVersion" or somesuch.
or maybe instead of trying to figure out what to name the method that returns the exclusive-upper-bound-version, have Version construct the VersionRange itself.
public VersionRange getCompatibleRange()There was a problem hiding this comment.
🤔 noted - have to think about this.
| public VersionRange versionRange() { | ||
| return new VersionRange(getMinVersion(), getMaxVersion()); | ||
| } | ||
|
|
||
| public Predicate<Version> versionPredicate() { |
There was a problem hiding this comment.
Thinking about types here.
Predicate<Version> is sufficient for gestalt ResolutionAttempt to do its thing. But there are a few other requirements we should consider:
- for troubleshooting purposes, this should at least have a
toStringvalue that makes more sense thanlambda@123or whatever. (Give feedback on failed dependency resolution #15 might also appreciate a rich type, but will probably find a string sufficient.) - for gradle compatibility, it needs either this minVersion, maxVersion pair or a string containing a maven-compatible range descriptor (which is what VersionRange.toString currently provides)
which makes me think we want to keep the return type as VersionRange after all.
There was a problem hiding this comment.
Let me bring up my question about sum types again... 🙄 nargh
There was a problem hiding this comment.
I don't think this one needs sum types. One class can implement both Predicate<Version> and ConvertableToMavenRange or whatever.
|
@keturn are any of the remaining comments blocking for this PR? If not, let's merge it and continue the discussion on |
| */ | ||
| public VersionRange versionRange() { |
There was a problem hiding this comment.
@keturn this is actually used in the Terasology modules builds 🙈 Guess we need to bring this method back to make that work again.
Closes #113