-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 run-operation to click CLI (#5552) #6656
Conversation
7f26774
to
4878009
Compare
core/dbt/cli/main.py
Outdated
@@ -349,6 +350,7 @@ def run(ctx, **kwargs): | |||
|
|||
# dbt run operation | |||
@cli.command("run-operation") | |||
@click.argument("macro") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tiny nit: could we move this down under @click.pass_context
so that all the params are logically grouped together?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also - we should probably define this in core/dbt/cli/params.py
alongside the other params for consistency / reuse.
edit, upon a bit more thinking:
A downside to that is that it wont be apparent from the command definition which parameters are arguments vs options. Also, I could see an argument for defining commands and arguments together given arguments can't provide their own help text and are documented by the command.
Alls to say - I think defining the argument here is actually fine, especially given that macros
is only used in one command at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with first nit! I'll make the order match the CLI order
For the second - two reasons I prefer keeping it alongside the command definitions:
- Click arguments can't have documentation / help text, click's recommendation is to document them by mentioning them explicitly in the command help text
- We have very very few positional arguments in dbt commands. (I think it's just
run-operation MACRO
andinit PROJECT_NAME
. I just remembered the latter, and will add in this PR so we don't forget.) These are truly one-offs. IMO adding them as (reusable) params risks the misperception that they're widely used, versus keeping them alongside the only commands for which they're relevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a small nit otherwise LGTM! thank you for picking this up!
2e7f400
to
b197bb6
Compare
resolves #5552
Sorry, just couldn't let others have all the fun...
L'engagement
RunOperationTask
to click CLImacro
positional argumentLe tour
Borrowing 63870d5 locally, to avoid the error
Object of type SpawnContext is not JSON serializable
:🎩 Le prestige
Checklist
changie new
to create a changelog entry