Note to myself if we end up with this version: I'd like to propose to tweak it just a bit further:

• for consistency within opam, consider keeping the same names as in the opam library used for operators (for example, <= would be "--leq", not "--le").
• I think it's fine (and probably convenient too) to include more operators (all?) rather than limiting to a specific subset. For example, "not equal" (--neq) can probably be useful.

For reference, here are the operators used in opam:

type relop = [`Eq|`Neq|`Geq|`Gt|`Leq|`Lt]

Edit: I added all operators systematically.

@kit-ty-kate I discovered the OpamArg.mk_* util, along with the validity of flags, and tried to use that. I think the code is missing the upcoming version 2.4 if I am understanding this correctly.

Yes you're correct, i forgot to add it when i bumped the version to 2.4. You can add it by simply adding the missing OpamArg{,Tools}.cli2_4, this is a simple one liner

Yes you're correct, i forgot to add it when i bumped the version to 2.4. You can add it by simply adding the missing OpamArg{,Tools}.cli2_4, this is a simple one liner

Done, Thanks!

Thank you @kit-ty-kate for the review and addressing these review comments!

I've pushed some improvement and fixes for the PR, as fixup commits. If it is ok for everyone, squash and good to go (after a rebase).

@rjbou Thanks for looking into the PR!

Re: Global options. My impression was that some might be applicable. Consider for example:

      --cli=MAJOR.MINOR (absent=2.4)
           Use the command-line interface syntax and semantics of MAJOR.MINOR.
           Intended for any persistent use of opam (scripts, blog posts,
           etc.), any version of opam in the same MAJOR series will behave as
           for the specified MINOR release. The flag was not available in opam
           2.0, so to select the 2.0 CLI, set the OPAMCLI environment variable
           to 2.0 instead of using this parameter.

Is --cli controlled by the global options? If so, I don't understand the purpose of setting the cli version in the code, if the flag cannot be used (in the script version of the command). I don't really understand how this works, so please let me know!

Re: Inlining OpamFormula.all_relop - I am obviously not opposed, but I will just mentioned, as a regular user of ppx_enumerate, it makes more sense to me to have the list of constructors be defined close to the type, and that being re-usable. Do you want to explicitly vet any new constructor for inclusion in the compare-versions command?

Is --cli controlled by the global options? If so, I don't understand the purpose of setting the cli version in the code, if the flag cannot be used (in the script version of the command). I don't really understand how this works, so please let me know!

that option is controlled by the user. In this particular case, the use of cli is simply to say "this option is available only since opam 2.4" and will show an error if a user says opam admin compare-versions --cli=2.0 or something like that. This is only useful for backward compatibility (renamed cli, some behaviour changes, …).

Re: Inlining OpamFormula.all_relop - I am obviously not opposed, but I will just mentioned, as a regular user of ppx_enumerate, it makes more sense to me to have the list of constructors be defined close to the type, and that being re-usable. Do you want to explicitly vet any new constructor for inclusion in the compare-versions command?

I do agree that i personally prefer the previous version as well for the same reason. @rjbou what is the reason for changing this part of the code?

Is --cli controlled by the global options? If so, I don't understand the purpose of setting the cli version in the code, if the flag cannot be used (in the script version of the command). I don't really understand how this works, so please let me know!

It's a mistake in the manpage, cli option (flag and environment variable) is special, it is preprocessed. The flag should be in the COMMON OPTIONS section.

Re: Inlining OpamFormula.all_relop - I am obviously not opposed, but I will just mentioned, as a regular user of ppx_enumerate, it makes more sense to me to have the list of constructors be defined close to the type, and that being re-usable. Do you want to explicitly vet any new constructor for inclusion in the compare-versions command?

You got a point here! Ignore my last commit.

Is --cli controlled by the global options? If so, I don't understand the purpose of setting the cli version in the code, if the flag cannot be used (in the script version of the command). I don't really understand how this works, so please let me know!

It's a mistake in the manpage, cli option (flag and environment variable) is special, it is preprocessed. The flag should be in the COMMON OPTIONS section.

If I understand correclty, you are saying that even though the flag doesn't show up in the --help, it is still there. I just tried this by the way:

$ ./opam admin compare --cli 2.3 1.2 1.3
opam admin: compare-versions was added in version 2.4 of the opam CLI, but version 2.3 has been requested, which is older.
$ ./opam admin compare --cli 2.4 1.2 1.3
[ERROR] opam command-line version 2.4 is not supported.

Is the change related to 2.4 from the PR standalone, or are there other steps required to make this work locally?

You got a point here! Ignore my last commit.

About the commits from the PR, to make sure I don't do anything unexpected:

1. am I the one who does the rebase -i and:
2. to confirm, I will change the commits whose messages are "!fixup ..." or "fixup! ..." into fixup commits? (is there a semantic difference between "fixup!" and "!fixup"?)

am I the one who does the rebase -i and:

no no, don't worry. We are the ones who usually do that. The "squash and good to go" wasn't meant to be a directive, only a todo for Raja herself. The (auto)squash step will simply eliminate the fixup commits (aka. merge them into the previous commit). See man git-rebase. My !fixup commit is a ""typo"" although in this case it doesn't really matter, it's only meant to signal that this commit will be eliminated into the previous one(s) so we'll see it during the manual rebase anyway.

huhu, i didn't see that you answered @kit-ty-kate.

Is the change related to 2.4 from the PR standalone, or are there other steps required to make this work locally?

There was a missing addition to have the new cli version fully functional (see new commit fixup). It worth exporting that in its own PR. if you want to do it, go for it, otherwise, will do.

am I the one who does the rebase -i

The PR branch with fixup commits is a kind of opam team internal mechanism to show to others what are the changes proposed. I'll do the cleanup once we converge.

huhu, i didn't see that you answered @kit-ty-kate.

twice xD you're too rapid for me ^^

The "squash and good to go" wasn't meant to be a directive, only a todo for Raja herself

I confirm!

Sharing some thoughts
In fact, some of my fixup! xxx messages aren't real fixup commits. To have commits autosquashed with git rebase -i --autosquash it needs to have as commit message fixup! XXX where XXX is exactly the first line of the commit message to squash with. My xxx text was intended to be informative, I've put the commits after the ones to squash with. So I used a mixup :)

The (auto)squash step will simply eliminate the fixup commits (aka. merge them into the previous commit). See man git-rebase.

Thanks a lot for teaching me about git rebase --autosquash 😃

There was a missing addition to have the new cli version fully functional (see new commit fixup). It worth exporting that in its own PR. if you want to do it, go for it, otherwise, will do.

I just saw your "fixup! fixup! Add cli version for 2.4". Beware, if you say "fixup!" three times in a row, who knows what may happen! 😃

I guess when you do the rebase, you can reorder these cli commit first, so you'll be able to create the PR from an intermediate commit.

Thanks a lot!

@mbarbin if you're happy with my proposed changes, I can begin the finalisation.

@mbarbin if you're happy with my proposed changes, I can begin the finalisation.

This looks good to me. Thank you for the changes!

All good! Once #6268 merged, this one will be rebased (by us), and merged!

Thank you for the proposition, the (multiple) implementation proposals!

Thanks!

Armoury+DRA@Armoury /cygdrive/c/Devel/msvs-tools
$ opam admin compare-version 0.8 '0.8.0~dev'
0.8 < 0.8.0~dev

Happened to be on a machine which has opam 2.4~dev as its working opam copy - sometimes it's the little things, thank you for this 🥰