DM-26415: remove chained commands from pipetask2 and DM-26388: pipetask2 --butler-config should accept a repository URI #67
Conversation
Vestigial. Should have been removed before previous merge to master.
Recommendation from prevous review, simplifies passing of bucket-of-arguments.
Also removes "option forwarding" support, which was for chaned commands. Removes test_cliCmd.py because the only substantive test it did was for forwarding options.
| "pipeline. This must be the fully qualified class name.") | ||
|
|
||
|
|
||
| class OptionGroup(): |
There was a problem hiding this comment.
Did you want to add base class inside parentheses?
There was a problem hiding this comment.
No, but I guess I can remove the parens?
| return f | ||
|
|
||
|
|
||
| class pipeline_build_options(OptionGroup): # noqa: N801 |
There was a problem hiding this comment.
Short one-line docstring could be useful for these classes.
| ctrlMpExecOpts.skip_existing_option(), | ||
| ctrlMpExecOpts.save_qgraph_option(), | ||
| ctrlMpExecOpts.save_single_quanta_option(), | ||
| ctrlMpExecOpts.qgraph_dot_option()] |
There was a problem hiding this comment.
Nitpick - there is a mixture of wrapping/indentation styles in different classes like
self.decorators = [decorator1,
decorator2]
vs.
self.decorators = [
decorator1,
decorator2
]
I personally prefer latter style, feels more readable as it lets you pack longer lines without more wrapping.
There was a problem hiding this comment.
Oh, latter is nice. 👍
| kwargs : `dict` [`str`, `str`] | ||
| Ignored; click commands may accept options for more than one script | ||
| function and pass all the option kwargs to each of the script functions | ||
| which ingore these unused kwargs. |
There was a problem hiding this comment.
Hmm, this is potentially problematic, if for example client misspells option/keyword name, you won't catch it here. Could we do something safer here (and other places like this)?
There was a problem hiding this comment.
@andy-slac I think there is not a good reason for the explicit kwargs to have default values. What if I removed them? That way if any of them are misspelled, and therefore missing, there will be an error.
There was a problem hiding this comment.
(Click generates default value for all the options, the command function gets called with a kwarg for each of the @options it is decorated with.)
There was a problem hiding this comment.
Having default values here and in click indeed bothers me too. If we say that all arguments are required than indeed we can drop defaults here and that should reduce the probability for spelling errors.
Make 'qgraph' call 'build', and take its subcommands. Make 'run' call 'build' and 'qgraph', and take their subcommands. Groups options functionally so that commands that accept options for a function, like managing a butler, can share the same set of options.
it needs to accept any path or URI, per DM-26388
n8pease
force-pushed
the
tickets/DM-26415
branch
from
c97fef8
to
431b9a6
Compare
No description provided.