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 patttern #6186

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

arozovyk
Copy link

@arozovyk arozovyk commented Sep 2, 2024

Adresses #6152

@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 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
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants