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

[CT-1947] Alias --models to --select for all commands except dbt ls #6787

Merged
merged 3 commits into from
Jan 31, 2023

Conversation

aranke
Copy link
Member

@aranke aranke commented Jan 31, 2023

resolves #6782

Description

Checklist

@cla-bot cla-bot bot added the cla:yes label Jan 31, 2023
@aranke aranke changed the base branch from main to feature/click-cli January 31, 2023 02:48
@aranke
Copy link
Member Author

aranke commented Jan 31, 2023

Crazy to think that a few line changes have allowed nearly 100(!) additional tests to pass.

Before: 134 failed, 444 passed
After: 43 failed, 535 passed

@aranke aranke marked this pull request as ready for review January 31, 2023 03:09
@aranke aranke requested a review from a team as a code owner January 31, 2023 03:09
@aranke aranke requested review from nathaniel-may, ChenyuLInx, MichelleArk and iknox-fa and removed request for nathaniel-may January 31, 2023 03:09
Copy link
Contributor

@ChenyuLInx ChenyuLInx left a comment

Choose a reason for hiding this comment

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

LGTM.

# See https://github.com/dbt-labs/dbt-core/pull/6774#issuecomment-1408476095 for more info.
models = click.option(*model_decls, **select_attrs)
raw_select = click.option(*select_decls, **select_attrs)
select = click.option(*select_decls, *model_decls, **select_attrs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this is simpler code but slightly harder to read compared to just write the options here.

@MichelleArk
Copy link
Contributor

Did some manual testing by locally overwriting a command that accepts the new select (--select, --model) alias command with print(ctx.obj["flags"].SELECT) and found some issues when passing --select and --model to a command together.

The options are not treated as mutually exclusive via MultiOption and the resulting flags.SELECT value is clobbered by the value from the last alias option:

❯ python3 core/dbt/cli/main.py docs generate --select model_a model_b  --model model_c
('model_c',)

❯ python3 core/dbt/cli/main.py docs generate --model model_c --select model_a model_b
('model_a', 'model_b')

This was unexpected behaviour to me, but for better or for worse this is also how main's parsing currently works (printing args.select and exiting on main to test):

❯ dbt docs generate --select model_a model_b --model model_c
['model_c']
❯ dbt docs generate --select model_a model_b --model model_c
['model_c']
❯ dbt docs generate --model model_c --select model_a model_b 
['model_a', 'model_b']

We should probably open an issue to document this behaviour and investigate a fix that gets us --select/--model option mutual exclusivity if possible. But given that it's the current behaviour on main I don't see it as pressing to fix as part of this PR.

@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.

@aranke aranke closed this Jan 31, 2023
@aranke aranke reopened this Jan 31, 2023
@aranke aranke deleted the ct-1947 branch July 20, 2023 14:45
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.

4 participants