Skip to content

feat!: make version comparison semver compliant #115

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
May 6, 2021
Merged

feat!: make version comparison semver compliant #115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fbf9c30
test: adjust tests to exclude own snapshot version
jdrueckert May 4, 2021
9acfbf1
feat: add Version::getVersionCore
jdrueckert May 4, 2021
746a9d9
feat: base DependencyInfo::versionPredicate purely on VersionRange; e…
jdrueckert May 4, 2021
f4b5ab7
test: add tests for excluding next SNAPSHOT
jdrueckert May 4, 2021
5d01e21
chore: remove unused SemverExpression
jdrueckert May 4, 2021
f41d2f1
refactor: rename getVersionCore to getCoreVersion; clean up dosctring
skaldarnar May 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 4 additions & 20 deletions ...dule/src/main/java/org/terasology/gestalt/module/dependencyresolution/DependencyInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
package org.terasology.gestalt.module.dependencyresolution;

import org.terasology.gestalt.naming.Name;
import org.terasology.gestalt.naming.SemverExpression;
import org.terasology.gestalt.naming.Version;
import org.terasology.gestalt.naming.VersionRange;

Expand Down Expand Up @@ -73,9 +72,9 @@ public void setMinVersion(Version value) {
public Version getMaxVersion() {
if (maxVersion == null) {
if (minVersion.getMajor() == 0) {
return minVersion.getNextMinorVersion();
return minVersion.getCoreVersion().getNextMinorVersion();
}
return minVersion.getNextMajorVersion();
return minVersion.getCoreVersion().getNextMajorVersion();
}
return maxVersion;
}
Expand Down Expand Up @@ -109,25 +108,10 @@ public void setOptional(boolean optional) {
}

/**
* @return The range of supported versions
* Returns a predicate that yields true when applied to version that is within the version range described by this dependency information
*/
public VersionRange versionRange() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keturn this is actually used in the Terasology modules builds 🙈 Guess we need to bring this method back to make that work again.

return new VersionRange(getMinVersion(), getMaxVersion());
}

public Predicate<Version> versionPredicate() {
Copy link
Member

@keturn keturn May 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 toString value that makes more sense than lambda@123 or 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me bring up my question about sum types again... 🙄 nargh

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this one needs sum types. One class can implement both Predicate<Version> and ConvertableToMavenRange or whatever.

if (maxVersion != null) {
return versionRange();
} else {
String source;
if (minVersion.isSnapshot()) {
// semver doesn't like snapshots in caret ranges?
source = minVersion + " | ^" + minVersion.getNextPatchVersion();
} else {
source = "^" + minVersion;
}
return new SemverExpression(source);
}
return new VersionRange(getMinVersion(), getMaxVersion());
}

@Override
Expand Down
32 changes: 0 additions & 32 deletions gestalt-module/src/main/java/org/terasology/gestalt/naming/SemverExpression.java
View file
Open in desktop

This file was deleted.

7 changes: 7 additions & 0 deletions gestalt-module/src/main/java/org/terasology/gestalt/naming/Version.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,13 @@ public Version getNextPatchVersion() {
return new Version(semver.incrementPatchVersion());
}

/**
* Returns the version core according to semver, i.e. the purely numerical version without any pre-release or build metadata info
*/
public Version getCoreVersion() {
return new Version(semver.getNormalVersion());
}

@Override
public boolean equals(Object obj) {
if (this == obj) {
Expand Down
2 changes: 1 addition & 1 deletion gestalt-module/src/main/java/org/terasology/gestalt/naming/VersionRange.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

@Override
Expand Down
6 changes: 5 additions & 1 deletion .../src/test/java/org/terasology/gestalt/module/dependencyresolution/DependencyInfoTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ void testMinimumVersionUnderOnePointZero() {
assertTrue(predicate.test(new Version(nextVersion + "-SNAPSHOT")), "failed to match the next snapshot");

assertFalse(predicate.test(new Version("0.2.3")), "inappropriately matched an earlier patch version");

assertFalse(predicate.test(new Version("0.2.4-SNAPSHOT")), "inappropriately matched own snapshot version");
assertFalse(predicate.test(nextVersion.getNextMinorVersion().getSnapshot()), "inappropriately matched next 0.x minor snapshot");
assertFalse(predicate.test(nextVersion.getNextMinorVersion()), "inappropriately matched next 0.x minor version");
}

Expand All @@ -51,6 +52,8 @@ void testMinimumVersionOverOnePointZero() {
assertTrue(predicate.test(new Version(nextMinorVersion + "-SNAPSHOT")), "failed to match the next minor snapshot");

assertFalse(predicate.test(new Version("1.4.6")), "inappropriately matched an earlier patch version");
assertFalse(predicate.test(new Version("1.4.7-SNAPSHOT")), "inappropriately matched own snapshot version");
assertFalse(predicate.test(nextVersion.getNextMajorVersion().getSnapshot()), "inappropriately matched next major snapshot");
assertFalse(predicate.test(nextVersion.getNextMajorVersion()), "inappropriately matched next major version");
}

Expand All @@ -69,6 +72,7 @@ void testMinimumVersionIsSnapshot() {
Version nextVersion = release.getNextPatchVersion();
assertTrue(predicate.test(nextVersion), "failed to match the next patch version");
assertTrue(predicate.test(new Version(nextVersion + "-SNAPSHOT")), "failed to match the next snapshot");
assertFalse(predicate.test(release.getNextMajorVersion().getSnapshot()), "inappropriately matched next major snapshot");
assertFalse(predicate.test(release.getNextMajorVersion()), "inappropriately matched next major version");
}
}
4 changes: 2 additions & 2 deletions gestalt-module/src/test/java/org/terasology/gestalt/naming/VersionRangeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ public void errorIfUpperBoundLowerThanLowerBound() {
}

@Test
public void lowerSnapshotInRange() {
assertTrue(range.contains(new Version("1.2.3-SNAPSHOT")));
public void lowerSnapshotOutOfRange() {
assertFalse(range.contains(new Version("1.2.3-SNAPSHOT")));
}

@Test
Expand Down