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

mutually exclusive handling for warn_error_options and warn_error params in Click CLI #6771

Merged
merged 5 commits into from
Jan 30, 2023

Conversation

MichelleArk
Copy link
Contributor

@MichelleArk MichelleArk commented Jan 27, 2023

resolves #6579

Description

Checklist

  • I have read the contributing guide and understand what's expected of me
  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue

🎩
(warn_error_options set by user config, warn_error by CLI params)

# profiles.yml
config:
  warn_error_options:
    include:
      - PackageInstallPathDeprecation
❯ python3 core/dbt/cli/main.py --warn-error clean --project-dir ~/src/jaffle_shop
Usage: main.py clean [OPTIONS]
Try 'main.py clean -h' for help.

Error: warn_error_options: not allowed with argument warn_error

(both options set from CLI params)

❯ python3 core/dbt/cli/main.py --warn-error --warn-error-options '{"include": "all"}' clean --project-dir ~/src/jaffle_shop
Usage: main.py clean [OPTIONS]
Try 'main.py clean -h' for help.

Error: warn_error_options: not allowed with argument warn_error

@cla-bot cla-bot bot added the cla:yes label Jan 27, 2023
@MichelleArk MichelleArk changed the base branch from main to feature/click-cli January 27, 2023 23:11
@MichelleArk MichelleArk changed the title Ct 1798/warn error options click2 mutually exclusive handling for warn_error_options and warn_error params in Click CLI Jan 30, 2023
Ensure no elements from group are simultaneously provided by a user, as inferred from params_assigned_from_default.
Raises click.UsageError if any two elements from group are simultaneously provided by a user.
"""
set_flag = None
Copy link
Contributor Author

Choose a reason for hiding this comment

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

effectively a copy from main flags.py: https:/dbt-labs/dbt-core/blob/main/core/dbt/flags.py#L196

@MichelleArk MichelleArk reopened this Jan 30, 2023
@MichelleArk MichelleArk marked this pull request as ready for review January 30, 2023 14:29
@MichelleArk MichelleArk requested a review from a team January 30, 2023 14:29
@MichelleArk MichelleArk requested review from a team as code owners January 30, 2023 14:29
@github-actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@MichelleArk MichelleArk requested review from stu-k, iknox-fa and aranke and removed request for gshank January 30, 2023 15:54
Copy link
Member

@aranke aranke left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, left some comments around making the code more Pythonic.

core/dbt/cli/flags.py Show resolved Hide resolved
core/dbt/cli/flags.py Show resolved Hide resolved
core/dbt/flags.py Show resolved Hide resolved
test/unit/test_flags.py Show resolved Hide resolved
tests/unit/test_cli_flags.py Outdated Show resolved Hide resolved
Copy link
Member

@aranke aranke left a comment

Choose a reason for hiding this comment

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

Discussed comments offline, LGTM!

Flags(context, user_config)

@pytest.mark.parametrize("warn_error", ["True", "False"])
def test_mutually_exclusive_options_from_cli_and_envvar(self, warn_error, monkeypatch):
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably didn't have to have tests for some of these combos given that Click resolves the cli params and the env var params before we get to this stage in the code-- but I'll never complain about having more tests.

Copy link
Contributor

@iknox-fa iknox-fa left a comment

Choose a reason for hiding this comment

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

LGTM!

@MichelleArk MichelleArk merged commit acc88d4 into feature/click-cli Jan 30, 2023
@MichelleArk MichelleArk deleted the CT-1798/warn_error_options_click2 branch January 30, 2023 23:38
iknox-fa pushed a commit that referenced this pull request Jan 31, 2023
…ams in Click CLI (#6771)

warn_error_options, warn_error mutual exclusivity with click
stu-k pushed a commit that referenced this pull request Jan 31, 2023
…ams in Click CLI (#6771)

warn_error_options, warn_error mutual exclusivity with click
@aranke aranke mentioned this pull request Jan 31, 2023
6 tasks
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