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.

Sign up for GitHub

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

Jump to bottom

Allow --select and --exclude multiple times #7169

Merged
merged 7 commits into from
Mar 28, 2023
Merged

Allow --select and --exclude multiple times #7169

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
846e991
One argument per line
dbeatty10 Mar 14, 2023
2824f4a
Tests for multiple `--select` or `--exclude`
dbeatty10 Mar 14, 2023
4097afd
Allow `--select` and `--exclude` multiple times
dbeatty10 Mar 14, 2023
9da6e1a
Changelog entry
dbeatty10 Mar 14, 2023
f872a0d
MultiOption options must be specified with type=tuple or type=ChoiceT…
dbeatty10 Mar 15, 2023
cefd964
Testing for `--output-keys` and `--resource-type`
dbeatty10 Mar 15, 2023
33c6cc4
Validate that any new param with `MultiOption` should also have `type…
dbeatty10 Mar 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changes/unreleased/Breaking Changes-20230314-161505.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: Breaking Changes
body: Allow `--select` and `--exclude` multiple times
time: 2023-03-14T16:15:05.81741-06:00
custom:
Author: dbeatty10
Issue: "7158"
16 changes: 16 additions & 0 deletions core/dbt/cli/options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import click
import typing as t
from click import Context


# Implementation from: https://stackoverflow.com/a/48394004
Expand Down Expand Up @@ -42,3 +44,17 @@ def parser_process(value, state):
our_parser.process = parser_process
break
return retval

def type_cast_value(self, ctx: Context, value: t.Any) -> t.Any:
def flatten(data):
if isinstance(data, tuple):
for x in data:
yield from flatten(x)
else:
yield data

# there will be nested tuples to flatten when multiple=True
value = super(MultiOption, self).type_cast_value(ctx, value)
if value:
value = tuple(flatten(value))
Comment on lines +73 to +74
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know why, but the value can be None. We only want to flatten it when it has a non-None value.

return value
10 changes: 8 additions & 2 deletions core/dbt/cli/params.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,12 @@
)

exclude = click.option(
"--exclude", envvar=None, type=tuple, cls=MultiOption, help="Specify the nodes to exclude."
"--exclude",
envvar=None,
type=tuple,
cls=MultiOption,
multiple=True,
help="Specify the nodes to exclude.",
)

fail_fast = click.option(
Expand Down Expand Up @@ -178,7 +183,7 @@
"Space-delimited listing of node properties to include as custom keys for JSON output "
"(e.g. `--output json --output-keys name resource_type description`)"
),
type=list,
type=tuple,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Context

When I left this as type=list, a couple tests failed:

But when I updated it to type=tuple, they passed again:

Although it is not enforced, there is a comment here that says:

MultiOption options must be specified with type=tuple or type=ChoiceTuple

Suggestion

To avoid unforeseen issues in the future, maybe we should enforce that the configured type is one of the following whenever cls=MultiOption?

  • subclass of tuple
  • instance of ChoiceTuple

One option for enforcing this within the __init__ method of MultiOption:

option_type = kwargs.pop("type", None)
if inspect.isclass(option_type):
    assert issubclass(option_type, tuple), "type must be tuple or ChoiceTuple not {}".format(option_type)
else:
    assert isinstance(option_type, ChoiceTuple), "type must be tuple or ChoiceTuple not {}".format(option_type)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I implemented this suggestion in 33c6cc4

cls=MultiOption,
default=[],
)
Expand Down Expand Up @@ -314,6 +319,7 @@
"envvar": None,
"help": "Specify the nodes to include.",
"cls": MultiOption,
"multiple": True,
"type": tuple,
}

Expand Down
22 changes: 22 additions & 0 deletions tests/functional/graph_selection/test_graph_selection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,12 @@ def test_concat(self, project):
results = run_dbt(["run", "--select", "@emails_alt", "users_rollup"], expect_pass=False)
check_result_nodes_by_name(results, ["users_rollup", "users", "emails_alt"])

def test_concat_multiple(self, project):
results = run_dbt(
["run", "--select", "@emails_alt", "--select", "users_rollup"], expect_pass=False
)
check_result_nodes_by_name(results, ["users_rollup", "users", "emails_alt"])

def test_concat_exclude(self, project):
results = run_dbt(
[
Expand All @@ -177,6 +183,22 @@ def test_concat_exclude(self, project):
)
check_result_nodes_by_name(results, ["users_rollup", "users"])

def test_concat_exclude_multiple(self, project):
results = run_dbt(
[
"run",
"--select",
"@emails_alt",
"users_rollup",
"--exclude",
"users",
"--exclude",
"emails_alt",
],
expect_pass=False,
)
check_result_nodes_by_name(results, ["users_rollup"])

def test_concat_exclude_concat(self, project):
results = run_dbt(
[
Expand Down