opam-0install-cudf: Add support for the avoid-version flag #37
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kit-ty-kate
force-pushed
the
avoid-version-2
branch
2 times, most recently
from
3767312 to
d5676b8
Compare
dra27
reviewed
Apr 14, 2022
lib-cudf/opam_0install_cudf.ml
Outdated
Comment on lines
15
to
28
| let version_rev_compare context pkg1 pkg2 = | ||
| let rev_cmp () = | ||
| if context.prefer_oldest then | ||
| Int.compare pkg1.Cudf.version pkg2.Cudf.version | ||
| else | ||
| Int.compare pkg2.Cudf.version pkg1.Cudf.version | ||
| in | ||
| if context.handle_avoid_version then | ||
| match tagged_with_avoid_version pkg1, tagged_with_avoid_version pkg2 with | ||
| | true, true | false, false -> rev_cmp () | ||
| | true, false -> 1 | ||
| | false, true -> -1 | ||
| else | ||
| rev_cmp () |
There was a problem hiding this comment.
This function may be hot enough for it to be worth unrolling all four versions and selecting having:
let version_rev_compare context = if context.handle_avoid_version && context.prefer_oldest then version_rev_compare_avoid_oldest else (* ... *)but it's certainly worth removing the auxiliary function:
Suggested change
| let version_rev_compare context pkg1 pkg2 = | |
| let rev_cmp () = | |
| if context.prefer_oldest then | |
| Int.compare pkg1.Cudf.version pkg2.Cudf.version | |
| else | |
| Int.compare pkg2.Cudf.version pkg1.Cudf.version | |
| in | |
| if context.handle_avoid_version then | |
| match tagged_with_avoid_version pkg1, tagged_with_avoid_version pkg2 with | |
| | true, true | false, false -> rev_cmp () | |
| | true, false -> 1 | |
| | false, true -> -1 | |
| else | |
| rev_cmp () | |
| let version_rev_compare context = pkg1 pkg2 = | |
| let avoid_version_pkg1 = context.handle_avoid_version && tagged_with_avoid_version pkg1 in | |
| let avoid_version_pkg2 = context.handle_avoid_version && tagged_with_avoid_version pkg2 in | |
| if avoid_version_pkg1 <> avoid_version_pkg2 then | |
| if avoid_version_pkg1 then 1 else -1 | |
| else if context.prefer_oldest then | |
| Int.compare pkg1.Cudf.version pkg2.Cudf.version | |
| else | |
| Int.compare pkg2.Cudf.version pkg1.Cudf.version |
There was a problem hiding this comment.
done in ea571ba, what do you think?
There was a problem hiding this comment.
actually i improved it further, the commit linked above is outdated
lib-cudf/opam_0install_cudf.mli
Outdated
| @@ -6,16 +6,23 @@ type diagnostics | |||
|
|
|||
| val create : | |||
| ?prefer_oldest:bool -> | |||
| ?handle_avoid_version:bool -> (* TODO: Make it true by default on the next major breaking release *) | |||
There was a problem hiding this comment.
Do we have to wait for this? Or it simply that opam itself can opt-in to enabling it immediately? It feels like a bug that it was ignoring it before?
There was a problem hiding this comment.
I removed the parameter entirely
There was a problem hiding this comment.
i've put it back but made it true by default
kit-ty-kate
force-pushed
the
avoid-version-2
branch
from
738ed61 to
00b7d4f
Compare
kit-ty-kate
force-pushed
the
avoid-version-2
branch
from
ea571ba to
e72a6f6
Compare
kit-ty-kate
force-pushed
the
avoid-version-2
branch
from
e72a6f6 to
884aafa
Compare
kit-ty-kate
commented
Jul 30, 2024
kit-ty-kate
force-pushed
the
avoid-version-2
branch
from
884aafa to
6a02b51
Compare
kit-ty-kate
force-pushed
the
avoid-version-2
branch
from
6a02b51 to
76ffd4a
Compare
This was referenced Jul 30, 2024
|
Moved to ocaml-opam/opam-0install-cudf#2 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Take 2. Superseds #35
For some reason I hadn’t thought of this very simple solution in #35...