Improve `OrderableSlsVersion` model and simplify comparison logic #1183

aldexis · 2025-08-22T12:33:11Z

Before this PR

See #1179 and discussion starting from #1179 (review)

The comparison logic in VersionComparator is fairly convoluted, because we tried to order version types using priorities. But RELEASE_CANDIDATE is not universally ordered with respect to RELEASE_CANDIDATE_SNAPSHOT (for instance, 1.2.3-rc1 < 1.2.3-rc1-1-gabc < 1.2.3-rc2). This means you can't simply define a priority on version types that you can leverage to order the versions of different types.

Instead, we can simply model OrderableSlsVersion as having 5 components:

The already-existing major, minor and patch version numbers
RC version number
- When absent, it is considering as positive infinite for comparison purposes
Snapshot version number
- When absent, it is considering as negative infinite for comparison purposes

This leads to a simple lexicographic comparison of the different numbers, in order:

Version	Parsed
`1.0.0-rc1`	`[1, 0, 0, 1, -infinite]`
`1.0.0-rc1-1-gabcedf`	`[1, 0, 0, 1, 1]`
`1.0.0-rc1-2-gabcedf`	`[1, 0, 0, 1, 2]`
`1.0.0-rc2`	`[1, 0, 0, 2, -infinite]`
`1.0.0`	`[1, 0, 0, +infinite, -infinite]`
`1.0.0-1-gabcedf`	`[1, 0, 0, +infinite, 1]`
`1.0.0-2-gabcedf`	`[1, 0, 0, +infinite, 2]`
`1.0.1-rc1`	`[1, 0, 1, 1, -infinite]`
`1.0.1`	`[1, 0, 1, +infinite, -infinite]`
`1.1.0`	`[1, 1, 0, +infinite, -infinite]`
`2.0.0`	`[2, 0, 0, +infinite, -infinite]`

After this PR

==COMMIT_MSG==
Improve OrderableSlsVersion model and simplify comparison logic
==COMMIT_MSG==

Possible downsides?

changelog-app · 2025-08-22T12:33:15Z

Generate changelog in `changelog/@unreleased`

Type (Select exactly one)

Feature (Adding new functionality)
Improvement (Improving existing functionality)
Fix (Fixing an issue with existing functionality)
Break (Creating a new major version by breaking public APIs)
Deprecation (Removing functionality in a non-breaking way)
Migration (Automatically moving data/functionality to a new system)

Description

Improve OrderableSlsVersion model and simplify comparison logic

Check the box to generate changelog(s)

Generate changelog entry

aldexis · 2025-08-22T12:42:54Z

sls-versions/src/main/java/com/palantir/sls/versions/OrderableSlsVersion.java

+     * The release candidate version number, if this version is a release candidate or release candidate snapshot.
+     */
+    @Value.Derived
+    OptionalInt rcVersionNumber() {


Making this and the below intentionally package-private per #1179 (comment)

aldexis · 2025-08-22T12:52:46Z

sls-versions/src/main/java/com/palantir/sls/versions/VersionComparator.java

-import java.util.OptionalInt;

 /** Compares {@link OrderableSlsVersion}s by "newness", i.e., "1.4.0" is greater/newer/later than "1.2.1", etc.. */
 public enum VersionComparator implements Comparator<OrderableSlsVersion> {


Fwiw, at this point, this entire comparator could simply be:

Comparator.comparingInt(OrderableSlsVersion::getMajorVersionNumber) .thenComparingInt(OrderableSlsVersion::getMinorVersionNumber) .thenComparingInt(OrderableSlsVersion::getPatchVersionNumber) .thenComparingInt(version -> version.rcVersionNumber().orElse(Integer.MAX_VALUE)) .thenComparingInt(version -> version.snapshotVersionNumber().orElse(Integer.MIN_VALUE));

but I'm not sure if this would have a performance cost, and I can't make the VersionComparator.INSTANCE be this either

For widely used library code like this, it's almost certainly better to write this ourselves instead of using a comparator with a bunch of lambdas.

pkoenig10 · 2025-08-23T01:08:56Z

sls-versions/src/main/java/com/palantir/sls/versions/CompactVersion.java

-            distanceFromVersion = version.firstSequenceVersionNumber().orElse(0);
-        }
+        int rcNumber = version.rcVersionNumber().orElse(0);
+        int distanceFromVersion = version.snapshotVersionNumber().orElse(0);


Suggested change

int distanceFromVersion = version.snapshotVersionNumber().orElse(0);

int snapshotNumber = version.snapshotVersionNumber().orElse(0);

pkoenig10 · 2025-08-23T01:09:31Z

sls-versions/src/main/java/com/palantir/sls/versions/OrderableSlsVersion.java

+     * The release candidate version number, if this version is a release candidate or release candidate snapshot.
+     */
+    @Value.Derived
+    OptionalInt rcVersionNumber() {


These should probably go on SlsVersion - that's where all other fields are declared. Even ones that are only relevant for OrderableSlsVersion.

While that's true, these don't make sense outside of OrderableSlsVersion 🤔 If anything, I'd probably want to move down/remove the XXXsequenceVersionNumber methods from SlsVersion since they're never defined in NonOrderableSlsVersion

pkoenig10 · 2025-08-23T01:10:55Z

sls-versions/src/main/java/com/palantir/sls/versions/OrderableSlsVersion.java

+    /**
+     * The release candidate version number, if this version is a release candidate or release candidate snapshot.
+     */
+    @Value.Derived
+    OptionalInt rcVersionNumber() {


nit: It feels more correct to drop the version - these are just "numbers" not "version numbers". That's also consistent with verbiage we use elsewhere.

Suggested change

/**

* The release candidate version number, if this version is a release candidate or release candidate snapshot.

*/

@Value.Derived

OptionalInt rcVersionNumber() {

/**

* The release candidate number, if this version is a release candidate or release candidate snapshot.

*/

@Value.Derived

OptionalInt rcNumber() {

pkoenig10 · 2025-08-23T01:11:19Z

sls-versions/src/main/java/com/palantir/sls/versions/OrderableSlsVersion.java

+    /**
+     * The snapshot version number, if this version is a release snapshot or release candidate snapshot.
+     */
+    @Value.Derived
+    OptionalInt snapshotVersionNumber() {


Same here

Suggested change

/**

* The snapshot version number, if this version is a release snapshot or release candidate snapshot.

*/

@Value.Derived

OptionalInt snapshotVersionNumber() {

/**

* The snapshot number, if this version is a release snapshot or release candidate snapshot.

*/

@Value.Derived

OptionalInt snapshotNumber() {

I see in your example commit that you used snapshotVersion rather than snapshotNumber. Is there one you'd prefer? I'm even considering just snapshot itself fwiw

I like snapshotNumber. It's not a version but it is a number. snapshot sounds to vague.

pkoenig10 · 2025-08-23T01:11:53Z

sls-versions/src/main/java/com/palantir/sls/versions/OrderableSlsVersion.java

+        if (getType() == SlsVersionType.RELEASE_CANDIDATE || getType() == SlsVersionType.RELEASE_CANDIDATE_SNAPSHOT) {
+            // If the version is a release candidate (or RC snapshot),
+            //   the first sequence number is always the RC version number.
+            return firstSequenceVersionNumber();
+        }
+        return OptionalInt.empty();


Would prefer to use a switch without a default case to ensure we're considering every type. I think it reads a bit nicer too.

pkoenig10 · 2025-08-23T01:13:57Z

sls-versions/src/main/java/com/palantir/sls/versions/SlsVersion.java

    public abstract int getPatchVersionNumber();

+    /**
+     * The first version number after major, minor, and patch. Typically either RC or snapshot version number.


nits:

Avoid abbrevations

The "typically" seems unnecessary - we know exactly when this will be set.

Suggested change

* The first version number after major, minor, and patch. Typically either RC or snapshot version number.

* The first version number after major, minor, and patch. The release candidate number, if this version is a release candidate or a release candidate snapshot, and the snapshot number, if this version is a release snapshot.

We know exactly when this will be set.

That's not quite true, since this is a field on SlsVersion, not on OrderableSlsVersion. It's not quite determined that this must be empty if not an RC or a snapshot.

These fields will always be empty on a NonOrderableSlsVersion, right?

sls-version-java/sls-versions/src/main/java/com/palantir/sls/versions/NonOrderableSlsVersion.java

Lines 48 to 54 in bc4fc92

return Optional.of(new NonOrderableSlsVersion.Builder()

.value(value)

.majorVersionNumber(groups.groupAsInt(1))

.minorVersionNumber(groups.groupAsInt(2))

.patchVersionNumber(groups.groupAsInt(3))

.type(SlsVersionType.NON_ORDERABLE)

.build());

They will, but that's not part of the definition of SlsVersion. This is an implementation detail. I meant that semantically, nothing would prevent someone from building their own NonOrderableSlsVersion and setting these.

With that said, that isn't done as far as a I know, and I'm not sure how you'd really even do it in the first place, considering there isn't necessarily a sequence version number for non-orderable versions.

But that's why I believe these shouldn't live in SlsVersion in the first place and why I'd like to move rc and snapshot back down into OrderableSlsVersion (since they don't make sense in NonOrderableSlsVersion)

But that's why I believe these shouldn't live in SlsVersion in the first place and why I'd like to move rc and snapshot back down into OrderableSlsVersion (since they don't make sense in NonOrderableSlsVersion)

Sure that makes sense. Putting these values in SlsVersion in the first place was probably a mistake.

pkoenig10 · 2025-08-23T01:14:59Z

sls-versions/src/main/java/com/palantir/sls/versions/SlsVersion.java

    public abstract OptionalInt firstSequenceVersionNumber();

+    /**
+     * The second version number after major, minor, and patch. Typically snapshot version number for RC snapshots.


Suggested change

* The second version number after major, minor, and patch. Typically snapshot version number for RC snapshots.

* The first version number after major, minor, and patch. The snapshot number, if this version is a release candidate snapshot

pkoenig10 · 2025-08-23T01:17:42Z

sls-versions/src/main/java/com/palantir/sls/versions/VersionComparator.java

+        // If RC version is not present, treat it as meaning positive infinite,
+        //   as all releases/release snapshots are considered newer than any RC.


nit: Avoid abbreviations and ambiguous / usage

Suggested change

// If RC version is not present, treat it as meaning positive infinite,

// as all releases/release snapshots are considered newer than any RC.

// If release candidate version is not present, treat it as the maximum value.

// All release and release snapshots versions are newer than any release candidate or release candidate snapshot version.

pkoenig10 · 2025-08-23T01:19:34Z

sls-versions/src/main/java/com/palantir/sls/versions/VersionComparator.java

+        // If snapshot version is not present, treat it as meaning negative infinite,
+        //   as all snapshots are considered newer than the release/RC they're tied to.


Suggested change

// If snapshot version is not present, treat it as meaning negative infinite,

// as all snapshots are considered newer than the release/RC they're tied to.

// If snapshot version is not present, treat it as the minimum value.

// All release snapshots and release candidate snapshots versions are newer than their corresponding release and release candidate versions, respectively.

pkoenig10 · 2025-08-23T02:04:33Z

sls-versions/src/main/java/com/palantir/sls/versions/OrderableSlsVersion.java

+    /**
+     * The release candidate version number, if this version is a release candidate or release candidate snapshot.
+     */
+    @Value.Derived


I think we can do better than @Value.Derived.

Here's a commit that:

Makes the parsing logic a bit more explicit, allowing us to avoid @Value.Derived

Avoids builder allocations

Eliminates unnecessary fields from ImmutableNonOrderableSlsVersion

0069413

And I'd probably take this one step further and eliminate the firstSequenceVersionNumber and secondSequenceVersionNumber fields altogether. I'm not really concerned about performance if we're not using in an internal, potentially hot, code path like like the comparator - a single, trivial, non-allocating switch statement should be more than acceptable for an accessor method.

8543c49

I like this approach (and was able to verify that we're not degrading the parsing perf/allocations either). One thing to note though is that this introduces an ABI break by removing the builder methods for firstSequenceVersionNumber and secondSequenceVersionNumber (the first one is used in at least one project internally, though usage should be trivial to fix)

pkoenig10 · 2025-08-23T02:15:18Z

I'm on board with this change. I think I was hesitant to make a larger change to this API, but I think it's worthwhile to make the API a bit more sensible.

aldexis

Here are the JMH comparisons:
Develop

Benchmark                                           (versionString)  Mode  Cnt      Score      Error   Units
SlsVersionBenchmark.safeValueOf                             RELEASE  avgt    3     16.873 ±    0.898   ns/op
SlsVersionBenchmark.safeValueOf:gc.alloc.rate               RELEASE  avgt    3  12659.582 ±  666.309  MB/sec
SlsVersionBenchmark.safeValueOf:gc.alloc.rate.norm          RELEASE  avgt    3     56.000 ±    0.001    B/op
SlsVersionBenchmark.safeValueOf:gc.count                    RELEASE  avgt    3    187.000             counts
SlsVersionBenchmark.safeValueOf:gc.time                     RELEASE  avgt    3    106.000                 ms
SlsVersionBenchmark.safeValueOf                            SNAPSHOT  avgt    3    329.685 ±   78.529   ns/op
SlsVersionBenchmark.safeValueOf:gc.alloc.rate              SNAPSHOT  avgt    3   9349.215 ± 2210.692  MB/sec
SlsVersionBenchmark.safeValueOf:gc.alloc.rate.norm         SNAPSHOT  avgt    3    808.000 ±    0.001    B/op
SlsVersionBenchmark.safeValueOf:gc.count                   SNAPSHOT  avgt    3    193.000             counts
SlsVersionBenchmark.safeValueOf:gc.time                    SNAPSHOT  avgt    3    102.000                 ms
SlsVersionBenchmark.safeValueOf                                  RC  avgt    3    127.903 ±   15.892   ns/op
SlsVersionBenchmark.safeValueOf:gc.alloc.rate                    RC  avgt    3  10497.571 ± 1301.039  MB/sec
SlsVersionBenchmark.safeValueOf:gc.alloc.rate.norm               RC  avgt    3    352.000 ±    0.001    B/op
SlsVersionBenchmark.safeValueOf:gc.count                         RC  avgt    3    179.000             counts
SlsVersionBenchmark.safeValueOf:gc.time                          RC  avgt    3     93.000                 ms
SlsVersionBenchmark.safeValueOf                         RC_SNAPSHOT  avgt    3    276.389 ±    6.290   ns/op
SlsVersionBenchmark.safeValueOf:gc.alloc.rate           RC_SNAPSHOT  avgt    3   8059.568 ±  184.260  MB/sec
SlsVersionBenchmark.safeValueOf:gc.alloc.rate.norm      RC_SNAPSHOT  avgt    3    584.000 ±    0.001    B/op
SlsVersionBenchmark.safeValueOf:gc.count                RC_SNAPSHOT  avgt    3    198.000             counts
SlsVersionBenchmark.safeValueOf:gc.time                 RC_SNAPSHOT  avgt    3     96.000                 ms
SlsVersionBenchmark.safeValueOf                               DIRTY  avgt    3    362.697 ±    6.779   ns/op
SlsVersionBenchmark.safeValueOf:gc.alloc.rate                 DIRTY  avgt    3   7151.281 ±  132.109  MB/sec
SlsVersionBenchmark.safeValueOf:gc.alloc.rate.norm            DIRTY  avgt    3    680.000 ±    0.001    B/op
SlsVersionBenchmark.safeValueOf:gc.count                      DIRTY  avgt    3    176.000             counts
SlsVersionBenchmark.safeValueOf:gc.time                       DIRTY  avgt    3     85.000                 ms
SlsVersionBenchmark.safeValueOf                            DIRTY_RC  avgt    3    344.146 ±  336.398   ns/op
SlsVersionBenchmark.safeValueOf:gc.alloc.rate              DIRTY_RC  avgt    3   7547.803 ± 7256.864  MB/sec
SlsVersionBenchmark.safeValueOf:gc.alloc.rate.norm         DIRTY_RC  avgt    3    680.000 ±    0.001    B/op
SlsVersionBenchmark.safeValueOf:gc.count                   DIRTY_RC  avgt    3    178.000             counts
SlsVersionBenchmark.safeValueOf:gc.time                    DIRTY_RC  avgt    3     92.000                 ms

Current state of the PR:

Benchmark                                           (versionString)  Mode  Cnt      Score      Error   Units
SlsVersionBenchmark.safeValueOf                             RELEASE  avgt    3     16.798 ±    0.887   ns/op
SlsVersionBenchmark.safeValueOf:gc.alloc.rate               RELEASE  avgt    3  12715.520 ±  667.856  MB/sec
SlsVersionBenchmark.safeValueOf:gc.alloc.rate.norm          RELEASE  avgt    3     56.000 ±    0.001    B/op
SlsVersionBenchmark.safeValueOf:gc.count                    RELEASE  avgt    3    189.000             counts
SlsVersionBenchmark.safeValueOf:gc.time                     RELEASE  avgt    3    100.000                 ms
SlsVersionBenchmark.safeValueOf                            SNAPSHOT  avgt    3    270.576 ±   22.299   ns/op
SlsVersionBenchmark.safeValueOf:gc.alloc.rate              SNAPSHOT  avgt    3  11390.574 ±  935.650  MB/sec
SlsVersionBenchmark.safeValueOf:gc.alloc.rate.norm         SNAPSHOT  avgt    3    808.000 ±    0.001    B/op
SlsVersionBenchmark.safeValueOf:gc.count                   SNAPSHOT  avgt    3    194.000             counts
SlsVersionBenchmark.safeValueOf:gc.time                    SNAPSHOT  avgt    3    101.000                 ms
SlsVersionBenchmark.safeValueOf                                  RC  avgt    3    139.038 ±    3.726   ns/op
SlsVersionBenchmark.safeValueOf:gc.alloc.rate                    RC  avgt    3   9656.714 ±  257.133  MB/sec
SlsVersionBenchmark.safeValueOf:gc.alloc.rate.norm               RC  avgt    3    352.000 ±    0.001    B/op
SlsVersionBenchmark.safeValueOf:gc.count                         RC  avgt    3    194.000             counts
SlsVersionBenchmark.safeValueOf:gc.time                          RC  avgt    3     97.000                 ms
SlsVersionBenchmark.safeValueOf                         RC_SNAPSHOT  avgt    3    235.163 ±   38.654   ns/op
SlsVersionBenchmark.safeValueOf:gc.alloc.rate           RC_SNAPSHOT  avgt    3   9472.974 ± 1551.075  MB/sec
SlsVersionBenchmark.safeValueOf:gc.alloc.rate.norm      RC_SNAPSHOT  avgt    3    584.000 ±    0.001    B/op
SlsVersionBenchmark.safeValueOf:gc.count                RC_SNAPSHOT  avgt    3    161.000             counts
SlsVersionBenchmark.safeValueOf:gc.time                 RC_SNAPSHOT  avgt    3     86.000                 ms
SlsVersionBenchmark.safeValueOf                               DIRTY  avgt    3    247.986 ±   12.286   ns/op
SlsVersionBenchmark.safeValueOf:gc.alloc.rate                 DIRTY  avgt    3  10459.128 ±  520.746  MB/sec
SlsVersionBenchmark.safeValueOf:gc.alloc.rate.norm            DIRTY  avgt    3    680.000 ±    0.001    B/op
SlsVersionBenchmark.safeValueOf:gc.count                      DIRTY  avgt    3    215.000             counts
SlsVersionBenchmark.safeValueOf:gc.time                       DIRTY  avgt    3    110.000                 ms
SlsVersionBenchmark.safeValueOf                            DIRTY_RC  avgt    3    242.851 ±    1.067   ns/op
SlsVersionBenchmark.safeValueOf:gc.alloc.rate              DIRTY_RC  avgt    3  10680.313 ±   47.130  MB/sec
SlsVersionBenchmark.safeValueOf:gc.alloc.rate.norm         DIRTY_RC  avgt    3    680.000 ±    0.001    B/op
SlsVersionBenchmark.safeValueOf:gc.count                   DIRTY_RC  avgt    3    198.000             counts
SlsVersionBenchmark.safeValueOf:gc.time                    DIRTY_RC  avgt    3    101.000                 ms

aldexis · 2025-08-25T11:51:54Z

sls-versions/src/jmh/java/com/palantir/sls/versions/SlsVersionBenchmark.java

+        RC_SNAPSHOT("0.16.0-rc1-8-g116b425"),
+        DIRTY("0.16.0-8-g116b425.dirty"),
+        DIRTY_RC("0.16.0-rc1-8-g116b425.dirty"),


Made a change to the benchmark here, since RC_SNAPSHOT was actually a dirty version

aldexis · 2025-08-25T12:00:12Z

sls-versions/src/main/java/com/palantir/sls/versions/NonOrderableSlsVersion.java

+    @Override
+    public final OptionalInt rcNumber() {
+        return OptionalInt.empty();
+    }
+
+    @Override
+    public final OptionalInt snapshotNumber() {
+        return OptionalInt.empty();
+    }
+
+    @Override
+    public final SlsVersionType getType() {
+        return SlsVersionType.NON_ORDERABLE;
+    }


As mentioned, these introduce an ABI break on the builder type. Somehow rev-api doesn't seem to run, so I'll accept these and ensure rev-api runs in a separate PR

Same comment for OrderableSlsVersion below

Seems like the lack of revapi check was a bug in gradle-revapi, which was fixed on develop by migrating to https://github.com/revapi/gradle-revapi as part of #1184

I rebased the PR

aldexis · 2025-08-25T12:01:09Z

sls-versions/src/main/java/com/palantir/sls/versions/OrderableSlsVersion.java

-            if (groups != null) {
-                return Optional.of(construct(type, value, groups));
-            }
+        MatchResult groups = SlsVersionType.RELEASE.getParser().tryParse(value);


I'm not particularly fond of doing multiple parsings, when we could trivially extend the ReleaseVersionParser to parse everything for us (and be more efficient too).

I'm going to look into that in a separate follow-up PR

aldexis · 2025-08-25T12:01:52Z

sls-versions/src/main/java/com/palantir/sls/versions/SlsVersion.java

+    /**
+     * The release candidate version number, if this version is a release candidate or release candidate snapshot.
+     */
+    public abstract OptionalInt rcNumber();

-    public abstract OptionalInt secondSequenceVersionNumber();
+    /**
+     * The snapshot version number, if this version is a release snapshot or release candidate snapshot.
+     */
+    public abstract OptionalInt snapshotNumber();


This is a break too, for anybody extending SlsVersion, though as far as I can tell, this isn't done on any of our projects

aldexis · 2025-08-25T12:02:39Z

sls-versions/src/main/java/com/palantir/sls/versions/SlsVersionType.java

+    /**
+     * The priority was used to have a predetermined order of version types, to ease comparison,
+     *   but suffered problems due to RELEASE_CANDIDATE_SNAPSHOT and RELEASE_CANDIDATE versions not being systematically
+     *   ordered (e.g. 1.2.3-rc1 < 1.2.3-rc1-1-gabc < 1.2.3-rc2 < 1.2.3-rc2-1-gdef).
+     * @deprecated This field is deprecated and will be removed in a future release.
+     */
+    @Deprecated(forRemoval = true)


Might need to remove this if we're still concerned about deprecations blocking upgrades

aldexis · 2025-08-29T09:53:37Z

There are a couple of issues with this PR:

It is unfortunately breaking backwards compatibility with Java Serializability, as described in Explicitly set serialVersionUID and add serialization tests #1187 (comment)
- Trying to fix that is possible to some extent, but overly complex, and overall not worth the complexity
Using another approach by defining convenience methods relying on switches actually degrades perf, for a minimal benefit (see [DO NOT MERGE] Compare approaches to simplify version comparison #1191 for a detailed analysis)

In the end, the best tradeoff may actually be the original #1179. Ideally, we'd be able to improve the data model, but doing so has a cost on either speed or memory, so it's not particularly worth it, considering the approach in that other PR is still sensible

aldexis requested a review from pkoenig10 August 22, 2025 12:33

aldexis mentioned this pull request Aug 22, 2025

Use same priority for RC and RC snapshots #1179

Merged

aldexis commented Aug 22, 2025

View reviewed changes

pkoenig10 reviewed Aug 23, 2025

View reviewed changes

aldexis commented Aug 25, 2025

View reviewed changes

aldexis mentioned this pull request Aug 25, 2025

Streamline and improve performance for orderable sls version parsing #1185

Closed

aldexis added 4 commits August 25, 2025 15:41

Improve OrderableSlsVersion model and simplify comparison logic

bc4cdad

Add comments

4657707

Address comments

3024d91

More things to address

43cc394

aldexis force-pushed the ald/better-sls-version-comparison branch from 5067776 to 43cc394 Compare August 25, 2025 13:41

pkoenig10 mentioned this pull request Aug 26, 2025

Explicitly set serialVersionUID and add serialization tests #1187

Merged

This was referenced Aug 28, 2025

Add benchmark for comparing versions #1190

Merged

[DO NOT MERGE] Compare approaches to simplify version comparison #1191

Closed

aldexis closed this Aug 29, 2025

	int distanceFromVersion = version.snapshotVersionNumber().orElse(0);
	int snapshotNumber = version.snapshotVersionNumber().orElse(0);

	* The first version number after major, minor, and patch. Typically either RC or snapshot version number.
	* The first version number after major, minor, and patch. The release candidate number, if this version is a release candidate or a release candidate snapshot, and the snapshot number, if this version is a release snapshot.

	return Optional.of(new NonOrderableSlsVersion.Builder()
	.value(value)
	.majorVersionNumber(groups.groupAsInt(1))
	.minorVersionNumber(groups.groupAsInt(2))
	.patchVersionNumber(groups.groupAsInt(3))
	.type(SlsVersionType.NON_ORDERABLE)
	.build());

	* The second version number after major, minor, and patch. Typically snapshot version number for RC snapshots.
	* The first version number after major, minor, and patch. The snapshot number, if this version is a release candidate snapshot

		// If RC version is not present, treat it as meaning positive infinite,
		// as all releases/release snapshots are considered newer than any RC.

		// If snapshot version is not present, treat it as meaning negative infinite,
		// as all snapshots are considered newer than the release/RC they're tied to.

Improve OrderableSlsVersion model and simplify comparison logic #1183

Improve OrderableSlsVersion model and simplify comparison logic #1183

Uh oh!

Conversation

aldexis commented Aug 22, 2025

Before this PR

After this PR

Possible downsides?

Uh oh!

changelog-app bot commented Aug 22, 2025

Generate changelog in changelog/@unreleased

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aldexis Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pkoenig10 Aug 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pkoenig10 commented Aug 23, 2025

Uh oh!

aldexis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aldexis Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Improve `OrderableSlsVersion` model and simplify comparison logic #1183

Improve `OrderableSlsVersion` model and simplify comparison logic #1183

Generate changelog in `changelog/@unreleased`

aldexis Aug 25, 2025 •

edited

Loading

pkoenig10 Aug 23, 2025 •

edited

Loading

aldexis Aug 25, 2025 •

edited

Loading