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

Fix opam switch list-available pkg.version pattern #6186

Merged
merged 2 commits into from
Dec 3, 2024

Conversation

arozovyk
Copy link

@arozovyk arozovyk commented Sep 2, 2024

Adresses #6152
Queued on #6318

@arozovyk arozovyk force-pushed the list_available_pkg.version_pat branch 2 times, most recently from 1538737 to 32b9d00 Compare September 2, 2024 10:15
@arozovyk arozovyk marked this pull request as ready for review September 2, 2024 10:22
@kit-ty-kate kit-ty-kate requested a review from rjbou September 2, 2024 14:23
@kit-ty-kate kit-ty-kate added this to the 2.4.0~alpha1 milestone Sep 25, 2024
@rjbou rjbou force-pushed the list_available_pkg.version_pat branch from 7c83981 to 4a5dc46 Compare October 30, 2024 16:34
@rjbou rjbou requested a review from kit-ty-kate October 30, 2024 16:35
Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

Thanks! As we discussed, I've reworked the change.

Copy link
Member

@kit-ty-kate kit-ty-kate left a comment

Choose a reason for hiding this comment

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

I'm not sure ands made sense in general in the first place. I think we should change the behaviour to use ors instead.
Otherwise there is no use to allow several parameters. e.g.

$ opam switch list-available ocaml-base-compiler ocaml-variants
# Listing available compilers from repositories: kit-ty-kate, default
# No matches found

If we switch to ors instead we can also simply remove the addition of the ?concat, which doesn't seem useful to me.

Ideally we should also have a test showing the behaviour of list-available with several parameters

@rjbou
Copy link
Collaborator

rjbou commented Nov 29, 2024

If we switch from ands to ors, it is the purpose of another PR.

@kit-ty-kate kit-ty-kate added the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Nov 30, 2024
@kit-ty-kate
Copy link
Member

If we switch from ands to ors, it is the purpose of another PR.

Done in #6318. This should be merged first. I've marked this here PR as queued on it.

@kit-ty-kate kit-ty-kate changed the title Fix opam switch list-available pkg.version patttern Fix opam switch list-available pkg.version pattern Nov 30, 2024
@kit-ty-kate kit-ty-kate force-pushed the list_available_pkg.version_pat branch from 4a5dc46 to e45202e Compare November 30, 2024 23:47
@kit-ty-kate
Copy link
Member

rebased

@kit-ty-kate kit-ty-kate force-pushed the list_available_pkg.version_pat branch from e45202e to 06b9efd Compare November 30, 2024 23:56
@kit-ty-kate
Copy link
Member

new proposal queued on top of #6318 with one extra test

@rjbou rjbou force-pushed the list_available_pkg.version_pat branch from 06b9efd to 69a1fc5 Compare December 3, 2024 16:32
@rjbou rjbou added STATE: READY TO MERGE and removed PR: QUEUED Pending pull request, waiting for other work to be merged or closed labels Dec 3, 2024
@kit-ty-kate
Copy link
Member

Thanks!

@kit-ty-kate kit-ty-kate merged commit 8a38781 into ocaml:master Dec 3, 2024
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants