-
Notifications
You must be signed in to change notification settings - Fork 374
Add opam repo show <reponame> support
#6720
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
base: master
Are you sure you want to change the base?
Conversation
opam repo show .. supportopam repo show <reponame> support
opam repo show <reponame> supportopam repo show … support
|
(sorry, I can't get past |
opam repo show … supportopam repo show <reponame> support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR, i've pushed a commit to apply some of our more common coding style, so that the new code doesn't feel too out of place.
I've tried and it works reasonably well, however i've hit some issues:
$ opam repo show --switch=does-not-exist default
switch: does-not-exist
url: https://staging.opam.ocaml.org/
rank: 2
with does-not-exist being a non-existent switch and default being an existent repository.
I don't think this should return that kind of output (i would more expect something like an error).
I also think the new command should have a way to display the details of a repository that's not currently attached to any switches (maybe with --dont-select?)
I'm also wondering how useful would opam repo show -a be, but we could add it later if needed i suppose.
|
There are some other commands with the same behaviour ( $ opam repo list --switch=abc
[NOTE] These are the repositories in use by the current switch. Use '--all'
to see all configured repositories.
<><> Repository configuration for switch abc <><><><><><><><><><><><><><><><>
1 default https://opam.ocaml.orgReading more the code, I found that the actual flag to select switch is Because opam/src/client/opamCommands.ml Lines 2375 to 2383 in ce734b3
Could you please verify? |
|
Ah my bad, i thought I think we should fix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some more design questions:
- do we want
opam repo show does-no-existto exit 0 with no error message? - do we want an extra
name:line, even if a bit useless at the moment, it might be useful later ifopam repo show -ais added - do we want to have colors? This might help with readability (for example
opam show <pkg>has its fields coloured blue) - what is the intended way to parse the output? Wait for switch, then parse each line in this context until a new line starting with
switch:?
src/client/opamRepositoryCommand.ml
Outdated
| List.iter (fun (sw, repos) -> | ||
| List.iter (fun (rank, repo) -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is probably a better way to do this but i think it would be nice to have the different switches visually separated. opam repo show -a default is quite hard to look at, at the moment
| List.iter (fun (sw, repos) -> | |
| List.iter (fun (rank, repo) -> | |
| let did_print_atleast_once = ref false in | |
| List.iter (fun (sw, repos) -> | |
| List.iter (fun (rank, repo) -> | |
| if !did_print_atleast_once then | |
| OpamConsole.msg "\n" | |
| else | |
| did_print_atleast_once := true; |
|
Hey @kit-ty-kate, I made the above changes.
Yes, the output should be a It now works with |
Related to issue #6235.
Adding a new command that will allow us to get the URL and Rank of a specific repo.
Also supports
all(will display the url and rank of a repo for every switch):