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

Add a verbose-on option. #5682

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

Conversation

desumn
Copy link

@desumn desumn commented Sep 20, 2023

Implementation of #5331

Add a verbose-on=PACKAGES parameters to build options. For the time being, it's equivalent to a simple -v for a list of package.

I'm not sure how I'd be able to simulate a modification of the opam file of a specific package to change add the verbose flag, so it's not exactly what #5331 asked for, however if there's a way I'd be glad to implement it!

@rjbou rjbou added the PR: WIP Not for merge at this stage label Sep 20, 2023
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 a lot for this first PR!
I've put some comments, but the general idea on how you add it is good!
Please tell me if you need more precision!

src/client/opamAction.ml Outdated Show resolved Hide resolved
src/state/opamStateConfig.ml Outdated Show resolved Hide resolved
let verbose_on =
mk_opt ~cli (cli_from cli2_2) ~section ["verbose-on"] "PACKAGES"
"Be more verbose on specific packages."
Arg.(some (list package)) None ~vopt:(Some [])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here you have verbose_on and a name & version opt list, but only names are used. It'll be better to take only name list. You can use OpamArg.name_list

Suggested change
Arg.(some (list package)) None ~vopt:(Some [])
Arg.(some name_list) None ~vopt:(Some [])

src/client/opamClientConfig.mli Outdated Show resolved Hide resolved
src/state/opamStateConfig.ml Outdated Show resolved Hide resolved
@rjbou
Copy link
Collaborator

rjbou commented Sep 26, 2023

I've added a test (tests/reftests/verbose-on.test), like that you can it is possible to test your feature, and we'll keep it as a new test in the testsuite, to be sure to detect regression in PRs.

I've also rebased your PR, we prefer rebasing on master than merging master for PRs, it eases history/commits lookup.

@kit-ty-kate kit-ty-kate added PR: WAITING FOR REVIEW and removed PR: WIP Not for merge at this stage labels Oct 16, 2023
@desumn
Copy link
Author

desumn commented Dec 2, 2023

Thanks for your review, I did it a bit late... but I think there's nothing more to do about what you said, I'll however say I don't exactly know how to test env variables, and I'm not sure if what I did works (at least it compiles!)

@rjbou
Copy link
Collaborator

rjbou commented Dec 7, 2023

No worry! We'll try to review the changes in the next weeks. We are also focused on the 2.2 release on our side :)

@desumn
Copy link
Author

desumn commented Apr 12, 2024

It's been some time, do you have any update on this?

@rjbou
Copy link
Collaborator

rjbou commented Apr 12, 2024

Sorry for the delay, we are still diving into the release of 2.2 (that is in its beta2 since 2 days). I've put some comments, with these modification and an update of master_changes (points to add in Client, Reftests, and API), the PR should be nearly complete.

src/client/opamArg.ml Outdated Show resolved Hide resolved
src/client/opamArg.ml Outdated Show resolved Hide resolved
src/core/opamStd.ml Outdated Show resolved Hide resolved
desumn and others added 3 commits April 18, 2024 16:09
Co-authored-by: R. Boujbel <rjbou@ocamlpro.com>
Co-authored-by: R. Boujbel <rjbou@ocamlpro.com>
… its name)

Co-authored-by: R. Boujbel <rjbou@ocamlpro.com>
@desumn
Copy link
Author

desumn commented Apr 18, 2024

Thanks for your reviews, I commited the changes.

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.

None yet

3 participants