Repository navigation
Confusing process.release.compareVersion API #20053
Description
Activity
- addedmetaIssues and PRs related to the general management of the project.Issues and PRs related to the general management of the project.processIssues and PRs related to the process subsystem.Issues and PRs related to the process subsystem.
on Apr 15, 2018 @devsnek originally landed the API with the signum as you described in #19438, which was reverted after I expressed my concern that the signum might be counterintuitive (see #19574 (comment)):
The signum of the result of
compareVersionseems counter-intuitive to me. In most (if not all) frameworks, a comparator returns a negative result if the first argument (orthis) is considered to be "before" the second argument, and a positive result if the first argument (orthis) is considered to be "after" the second argument (see e.g. java.lang.Comparable for a single-argument comparison).compareVersioncontradicts this well-established norm. I would expectcompareVersion(10, 0, 0) >= 0to return whether this release is v10 or newer, which follows the norm and is intuitive, but #19438 implements the opposite.The following discussion was a bit difficult to follow as @devsnek edited his response about ten times, but in one of the first revisions he agreed with me.
Reacted by Ruben BridgewaterI suggest you change it, as the way I see it isn't apparently very popular. I'll approve whatever pr is opened 👍
- added a commit that references this issue
on Apr 16, 2018 Why is this even in core? This is the definition of feature creep, users should just be using the semver package to do this themselves rather than duplicating all that logic and prerelease rules.
The whole of #19587 is a smell, we're duplicating existing functionality (the numbers themselves! what's wrong with a
split()to get the major??) and functionality available via npm that includes some non-trivial rules that we need to make ensure is in sync (having dabbled in semver parsing and those rules I can say this is not straightforward).I'd +1 removing it entirely as a "fix", would I be alone in that?
proposing reversion @ #20062 along with some reasons and some notes on how the API is currently broken
The whole API got reverted. Closing therefore
It might only be me but the current implementation is not intuitive for me because I expect the output to be exactly the opposite of what it is. Even though it is not right or wrong either way.
Let's give an example.
The current master version is
10.0.0-pre.That way I expect
9.0.0to return-1because the input value is lower than the current version. Instead, it is1, because the current version is bigger than the input value. So it depends on what viewpoint the user is in.For me, it is more intuitive to have the input value being the main important part and not the current version (I hope it is clear what I want to express...).
So I would like to hear from others @nodejs/collaborators how they feel about this (it did not yet land on any release, so there would theoretically still be time to change this without being semver-major).