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

New operator '~' for semantic versioning #4532

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Keryan-dev
Copy link
Collaborator

@Keryan-dev Keryan-dev commented Feb 8, 2021

This follows #2976 as an attempt to implement the '~' operator. It requires another patch of opam-file-format.

Given the difference between this operator and the others, I tried to avoid it to go too deep in the opam internals. Thus, most of the changes are contained in opamFormat.ml where occurrences of the new operator are caught and rewrote from ~ v to >= v & < next(v) with the behavior of next defined in the issue.

I open the PR as a draft, as I feel the changes might need some more work to be satisfactory. Comments and reviews are most welcome.

@rjbou rjbou added this to the 2.2.0~alpha milestone Feb 10, 2021
@rjbou rjbou added the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Feb 10, 2021
@rjbou rjbou marked this pull request as ready for review July 30, 2021 10:05
@rjbou rjbou linked an issue Jul 30, 2021 that may be closed by this pull request
@rjbou rjbou removed the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Dec 1, 2021
@rjbou rjbou requested review from AltGr and rjbou December 1, 2021 19:44
@dra27
Copy link
Member

dra27 commented Feb 11, 2022

Notes from dev meeting:

  • The fact that older opam would silently interpret this as >= means we don't feel this can be reliably used in opam files
  • However, it would be very useful to be able to use it with --formula
  • Conclusion is to look at having this properly (in opam files as well) in opam 3.0

@dra27 dra27 removed this from the 2.2.0~alpha milestone Feb 11, 2022
@kit-ty-kate kit-ty-kate added the PR: WIP Not for merge at this stage label Feb 25, 2022
Copy link
Member

@AltGr AltGr left a comment

Choose a reason for hiding this comment

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

This deserves some tests in tests/reftests, but otherwise LGTM!

Comment on lines +309 to +315
| Relop (op,e,f) ->
(match op.pelem with
| `Sem -> (* rewrite `Sem case *)
FAnd (FOp (aux e, `Geq, aux f),
FOp (aux e, `Lt, FNext (aux f)))
| `Eq | `Neq | `Gt | `Geq | `Lt | `Leq as op ->
FOp (aux e, op, aux f))
Copy link
Member

Choose a reason for hiding this comment

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

Easier to read with a flattened match ?

Suggested change
| Relop (op,e,f) ->
(match op.pelem with
| `Sem -> (* rewrite `Sem case *)
FAnd (FOp (aux e, `Geq, aux f),
FOp (aux e, `Lt, FNext (aux f)))
| `Eq | `Neq | `Gt | `Geq | `Lt | `Leq as op ->
FOp (aux e, op, aux f))
| Relop ({pelem = `Sem; _}, e, f) -> (* rewrite `Sem case *)
FAnd (FOp (aux e, `Geq, aux f),
FOp (aux e, `Lt, FNext (aux f)))
| Relop ({pelem = `Eq | `Neq | `Gt | `Geq | `Lt | `Leq as op; _}, e, f) ->
FOp (aux e, op, aux f))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: WIP Not for merge at this stage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support for semver version constraint operator
5 participants