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

click working with list #6641

Merged
merged 14 commits into from
Jan 19, 2023
Merged

click working with list #6641

merged 14 commits into from
Jan 19, 2023

Conversation

MichelleArk
Copy link
Contributor

@MichelleArk MichelleArk commented Jan 18, 2023

resolves #5549

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
    Sanity tophatting 🎩
❯ python3 core/dbt/cli/main.py ls --project-dir ~/src/jaffle_shop
02:20:31  Found 5 models, 20 tests, 0 snapshots, 0 analyses, 303 macros, 0 operations, 3 seed files, 0 sources, 1 exposure, 0 metrics
exposure:jaffle_shop.weekly_jaffle_metrics
jaffle_shop.customers
jaffle_shop.orders
...

Using multiple multi-value options together:

> python3 core/dbt/cli/main.py ls --project-dir ~/src/jaffle_shop --select customers orders --resource-type model test --exclude customers not_null_orders_amount
21:27:55  Found 5 models, 20 tests, 0 snapshots, 0 analyses, 303 macros, 0 operations, 3 seed files, 0 sources, 1 exposure, 0 metrics
jaffle_shop.orders
jaffle_shop.accepted_values_orders_status__placed__shipped__completed__return_pending__returned
jaffle_shop.not_null_orders_bank_transfer_amount
...
  • This PR includes tests, or tests are not required/relevant for this PR. Manually found functional tests that rely on the ls command:
    • tests/functional/list/test_list.py
    • tests/functional/selectors/test_default_selectors.py::TestDefaultSelectors::test_model__list
    • tests/functional/graph_selection/test_graph_selection.py::TestGraphSelection::test_exposure_parents. Fails on Object of type SpawnContext is not JSON serializable
    • tests/functional/metrics/test_metric_deferral.py::TestMetricDeferral::test_metric_deferral. Fails on Object of type SpawnContext is not JSON serializable
  • I have opened an issue to add/update docs, or docs changes are not required/relevant for this PR
  • I have run changie new to create a changelog entry

@cla-bot cla-bot bot added the cla:yes label Jan 18, 2023
@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.


output = click.option(
"--output",
envvar=None,
help="TODO: No current help text",
type=click.Choice(["json", "name", "path", "selector"], case_sensitive=False),
default="name",
default="selector",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Matching default value of "selector" with what's in main.py on the main branch


# Implementation from: https://stackoverflow.com/a/48394004
# Note OptionEatAll options must be specified with type=tuple (https://github.com/pallets/click/issues/2012)
class OptionEatAll(click.Option):
Copy link
Contributor Author

@MichelleArk MichelleArk Jan 18, 2023

Choose a reason for hiding this comment

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

adding this to support backwards compatability for --select, --model, --exclude, and --resource-type options. click supports a multiple=True setting for options that expect multiple values to be set through multiple instances of the option, ie:
--select a --select b --select c, as opposed to our existing --select a b c

There is an option for setting nargs=-1 in click but this is limited to a single option, that is specified last in the command.

confirmed with @jtcohen6 that not supporting the --select a b c syntax would represent too large of a breaking change for a minor release.

@MichelleArk MichelleArk marked this pull request as ready for review January 18, 2023 21:35
@MichelleArk MichelleArk requested a review from a team as a code owner January 18, 2023 21:35
@MichelleArk MichelleArk reopened this Jan 18, 2023
@MichelleArk MichelleArk requested review from iknox-fa, stu-k, ChenyuLInx and aranke and removed request for gshank and emmyoop January 18, 2023 21:41
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.

I don't like the name OptionEatAll, but otherwise LGTM!

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.

Looks good to me! The new parse is great!

@jtcohen6 jtcohen6 linked an issue Jan 19, 2023 that may be closed by this pull request
@MichelleArk MichelleArk reopened this Jan 19, 2023
@MichelleArk MichelleArk merged commit 30a1595 into feature/click-cli Jan 19, 2023
@MichelleArk MichelleArk deleted the CT-925/click-dbt-list branch January 19, 2023 14:28
ChenyuLInx pushed a commit that referenced this pull request Jan 20, 2023
* list working with click
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.

[CT-925] dbt list works in click
4 participants