Skip to content
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.

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

We need a "compare" command independent of "order" #1347

Closed
krader1961 opened this issue Jun 28, 2021 · 6 comments · Fixed by #1435
Closed

We need a "compare" command independent of "order" #1347

krader1961 opened this issue Jun 28, 2021 · 6 comments · Fixed by #1435

Comments

@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Jun 28, 2021

I have been working on an Elvish way to compare values in order to implement semantic version comparisons. The default comparison function used by the order command appears to do what I want. It is a shame that I have to reinvent that wheel using Elvish commands.

@zzamboni
Copy link
Contributor

@zzamboni zzamboni commented Jun 28, 2021

@krader1961
Copy link
Contributor Author

@krader1961 krader1961 commented Jun 29, 2021

@zzamboni, Your implementation is pretty good but doesn't correctly handle prerelease and build metadata. See the spec and this solution which appears to correctly implement the spec.

@krader1961 krader1961 changed the title We need a "compare" function independent of "order" We need a "compare" command independent of "order" Jun 29, 2021
@krader1961
Copy link
Contributor Author

@krader1961 krader1961 commented Jun 29, 2021

@zzamboni, Also, your version doesn't correctly handle invalid semver strings. In particular, it is overly permissive by treating strings like "1.0.0" and "1.0" as equal. While at the same time not handling a very common, but incorrect, inclusion of "v" or "V" as a semver prefix. A mistake made by the current elvish implementation:

const Version = "v0.16.0"

I still think your module is awesome but it points out why a solution for the semver issue I opened is so important. Specifically, the semver project should include a file of test cases so that every implementation can be validated against that implementation agnostic set of test cases.

@zzamboni
Copy link
Contributor

@zzamboni zzamboni commented Jun 29, 2021

@krader1961 I think my implementation correctly handles prerelease strings, e.g. using the example from the SemVer spec:

'semver:<' 1.0.0-alpha  1.0.0-alpha.1  1.0.0-alpha.beta  1.0.0-beta 1.0.0-beta.2 1.0.0-beta.11 1.0.0-rc.1 1.0.0
▶ $true

See also a whole bunch of other tests at https://github.com/zzamboni/elvish-modules/blob/master/semver.org.

You are correct that a missing trailing numbers (MINOR or PATCH) are considered as zeros. In my defense, this is even documented 😉: https://github.com/zzamboni/elvish-modules/blob/master/semver.org#main-comparison-function. The spec says that all three components MUST be there, so I guess this is a bug. I've opened zzamboni/elvish-modules#17

Build metadata: this is also a bug, it should be ignored. I've created zzamboni/elvish-modules#16

Invalid things like the v at the beginning - what do you mean by "doesn't correctly handle"? You mean an error should be produced? At the moment this is allowed because the module uses string comparison when needed, and numeric otherwise. I suggest in zzamboni/elvish-modules#17 that it might make sense to have a &non-strict flag or variable which allows/disallows parsing things like that.

Thanks for the feedback. PR's welcome! 😉

@krader1961
Copy link
Contributor Author

@krader1961 krader1961 commented Jun 30, 2021

Invalid things like the v at the beginning - what do you mean by "doesn't correctly handle"?

@zzamboni, A "v" (or "V") prefix is technically invalid in a semver string. However, I would argue it should be silently ignored since it is all too common for an otherwise valid semver string to include that prefix. I didn't actually test that your implementation does so. I simply inferred it didn't from the documentation. If (or when) your implementation silently ignores a "v" prefix that is notable enough to warrant a mention in the documentation and why it is different from how every other invalid semver string is treated.

@zzamboni
Copy link
Contributor

@zzamboni zzamboni commented Jul 1, 2021

@krader1961 regarding your original issue, please note that as part of my semver module I implemented a compare function like the one you want. I agree this might be useful to have in Elvish:

The -signed-compare function compares two values using a function which takes two values and returns 1, 0 or -1 to represent the order of the two values (1: v1<v2, 0: v1==v2, -1: v1>v2)

fn -signed-compare [ltfn v1 v2]{
  util:cond [
    { $ltfn $v1 $v2 }  1
    { $ltfn $v2 $v1 } -1
    :else              0
  ]
}

About SemVer comparison, please check out zzamboni/elvish-modules#18, in which I address all the bugs mentioned above.

krader1961 added a commit to krader1961/elvish that referenced this issue Nov 22, 2021
Expose the default comparison function used by the `builtin:order`
command as a command in its own right. This command is useful when
writing, in Elvish, something like `builtin:order`. Such as a semantic
version comparison command.

Resolves elves#1347
@xiaq xiaq closed this in #1435 Dec 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants