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

Introduce `--confirm-level' option #4582

Merged
merged 11 commits into from
May 26, 2021
Merged

Introduce `--confirm-level' option #4582

merged 11 commits into from
May 26, 2021

Conversation

rjbou
Copy link
Collaborator

@rjbou rjbou commented Mar 4, 2021

Add --confirm-level to specify levels of automatic answer to opam questions, and --no

There is 3 levels:

  • unsafe-yes: yes to all questions & launch system package manager in non interactive mode
  • yes: answers no to all questions, similar to --yes
  • no: answers no to all questions, similar to --no
  • ask: ask for all questions

Those values can also be given to environment variable OPAMCONFIRMLEVEL.

Priorities:

  • On environment variables, take OPAMCONFIRMLEVEL if defined, then OPAMYES if true, then OPAMNO if true, kept undefined otherwise
  • On cli, --yes, --no, and --confirm-level can be repeated. Only the last of --yes or --no is taken into account, and only the last --confirm-level is taken into account. if --confirm-level and --yes/no are both given, --confirm-level content is taken.

@rjbou rjbou changed the title Introduce Confirm level Introduce `--confirm level' option Mar 4, 2021
@rjbou rjbou added PR: QUEUED Pending pull request, waiting for other work to be merged or closed PR: WIP Not for merge at this stage labels Mar 4, 2021
@rjbou rjbou changed the title Introduce `--confirm level' option Introduce `--confirm-level' option Mar 4, 2021
@rjbou rjbou linked an issue Mar 5, 2021 that may be closed by this pull request
@rjbou rjbou removed the PR: WIP Not for merge at this stage label Mar 31, 2021
@rjbou rjbou removed the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Apr 15, 2021
@dra27
Copy link
Member

dra27 commented Apr 16, 2021

From dev meeting:

  • Introduce OPAMCONFIRMLEVEL to take the value and have OPAMYES and OPAMNO remain booleans only
  • Need to remove OPAMDEPEXTYES either here or subsequently

src/client/opamArg.ml Outdated Show resolved Hide resolved
src/core/opamCoreConfig.ml Outdated Show resolved Hide resolved
Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

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

Priority of the environment variables looks good to me - a couple of questions about the handling of OPAMNO/OPAMCONFIRMLEVEL for plugins and a CLI question on conflicting options.

src/client/opamArg.ml Outdated Show resolved Hide resolved
src/core/opamCoreConfig.ml Outdated Show resolved Hide resolved
src/client/opamCliMain.ml Outdated Show resolved Hide resolved
src/core/opamConsole.ml Outdated Show resolved Hide resolved
src/client/opamArg.ml Outdated Show resolved Hide resolved
src/client/opamArg.ml Outdated Show resolved Hide resolved
Copy link
Member

@AltGr AltGr left a comment

Choose a reason for hiding this comment

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

LGTM
for mk_enum_opt_all I'll take your word on it 😬

Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

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

Some message change suggestions - I think it's clearer to state exactly how the env vars are interpreted

master_changes.md Show resolved Hide resolved
src/client/opamArg.ml Outdated Show resolved Hide resolved
src/client/opamArg.ml Outdated Show resolved Hide resolved
src/client/opamArg.ml Outdated Show resolved Hide resolved
src/client/opamCliMain.ml Outdated Show resolved Hide resolved
rjbou added 2 commits May 25, 2021 14:09
…to opam questions, and `--no`

There is 3 levels, 4 states:
* unsafe-yes: yes to all questions & launch system package manager in non interactive mode
* yes: answers no to all questions, similar to `--yes`
* no: answers no to all questions, similar to `--no`
* ask (default): ask for all questions
match E.confirmlevel (), E.yes (), E.no () with
| Some c, _, _ -> Some c
| _, Some true, _ -> Some `all_yes
| _, Some false, _
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
| _, Some false, _
| _, Some false, _ -> None

At present, OPAMYES=false opam ... still prompts, where this would still answer no.

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

Successfully merging this pull request may close these issues.

Command-line option needed for OPAMDEPEXTYES
4 participants