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 mapping options to CLI (e.g. --install-deps) #266

Closed
Nour-Mws opened this issue Mar 23, 2023 · 0 comments · Fixed by #341
Closed

Add mapping options to CLI (e.g. --install-deps) #266

Nour-Mws opened this issue Mar 23, 2023 · 0 comments · Fixed by #341
Assignees
Labels
P2 major: an upcoming release

Comments

@Nour-Mws
Copy link
Collaborator

This option is already usable via the toml config or env variables as per #252 but is still not available through the CLI.
This issue is to be tackled after some more progress and visibility have been obtained on #256.

Collecting here pieces of feedback from @jherland when I attempted to add the option in #252.

I don't think the default=False is needed when action="store_true" is used. Furthermore, I think it actually defeats the configuration cascade (which really depends on all relevant CLI options having default=argparse.SUPPRESS). What we want is that this behavior:

  • --install-deps on the command-line causes args.install_deps == True to be returned from argparse
  • No --install-deps on the command-line causes args.install_deps to NOT EXIST, allowing the install_deps option from the config file to take effect.

Additionally, I think the help message should emphasize that the deps are installed in a temporary/separate venv:

    parser.add_argument(
        "--install-deps",
        action="store_true",
        help=(
            "Allow FawltyDeps to `pip install` declared dependencies into a"
            " separate temporary virtualenv to discover the imports they expose"
        ),
    )

Originally posted by @jherland in #252 (comment)

This change highlights the problem that I talked about above: Setting install_deps = True in the config file, and then running without --install-deps on the command line should let the config file decide, but this illustrates that it doesn't: the default=False will always override whatever comes from the config file...

However, I realize there is an inherent problem with boolean command-line flags here: If you can only enable some behavior with a command-line option, then - if the behavior has already been enabled in the config file - how could you then disable this behavior at the command-line? I think the answer here is to provide a corresponding (mutually exclusive) --no-install-deps option that sets install_deps to False.

In other words we should have:

  • --install-deps: Set install_deps to True
  • --no-install-deps: Set install_deps to False
  • (nothing): Don't change install_deps at all. Use the value from the config file, falling back to the ultimate hardcoded default (False)

Originally posted by @jherland in #252 (comment)

@Nour-Mws Nour-Mws added the P2 major: an upcoming release label Mar 23, 2023
@Nour-Mws Nour-Mws added this to the Mapping strategy milestone Mar 23, 2023
@Nour-Mws Nour-Mws changed the title Add a --install-deps CLI option Add mapping options to CLI (e.g. --install-deps) Mar 30, 2023
@mknorps mknorps self-assigned this Apr 20, 2023
@mknorps mknorps self-assigned this Jul 25, 2023
@mknorps mknorps linked a pull request Jul 28, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 major: an upcoming release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants