-
Notifications
You must be signed in to change notification settings - Fork 128
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
Register subparsers recursively #1640
base: master
Are you sure you want to change the base?
Conversation
51f7dca
to
710f804
Compare
# Recursively register any subcommands | ||
if getattr(subparser, "subcommands", None): | ||
register_commands(subparser, subparser.subcommands) |
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.
This breaks augur curate
subcommands
$ cat records.ndjson | augur curate passthru
Traceback (most recent call last):
File "…/augur/__init__.py", line 72, in run
return args.__command__.run(args)
TypeError: run() missing 1 required positional argument: 'records'
because each subcommand's run
function is expected to be called through augur curate's own run
function, but it is now called directly at the top level.
Maybe recursively registering subcommands is not the right thing to do in this codebase.
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.
I remember having to work around the top level run when initially adding the augur curate
subcommand.
augur/augur/curate/__init__.py
Lines 117 to 120 in 129fd7e
# Using a subcommand attribute so subcommands are not directly | |
# run by top level Augur. Process I/O in `curate`` so individual | |
# subcommands do not have to worry about it. | |
add_command_subparsers(subparsers, SUBCOMMANDS, SUBCOMMAND_ATTRIBUTE) |
Would it be weird to keep augur curate
as a special case?
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.
Two options:
- Further adapt how
augur curate
commands share functionality so calling theirrun()
from the top-level works. This could be done relatively straightforwardly. - Don't recursively register subcommands; leave the pattern of commands with subcommands invoking
register_commands(…)
themselves.
I don't think either gets the way of the functional improvements we want to make? So it's a matter of preference.
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.
Thanks for the input – I'll try option 1 above to align with Nextstrain CLI code.
This is necessary to register subparsers recursively.
This is necessary to register subparsers recursively.
This information will be lost when registering subparsers recursively.
Keeps consistent with Nextstrain CLI.
Set a single source of truth.
This reverts commit 6581f10.
710f804
to
f5e9472
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1640 +/- ##
==========================================
+ Coverage 72.26% 72.36% +0.10%
==========================================
Files 79 80 +1
Lines 8271 8294 +23
Branches 1691 1691
==========================================
+ Hits 5977 6002 +25
+ Misses 2009 2008 -1
+ Partials 285 284 -1 ☔ View full report in Codecov by Sentry. |
Description of proposed changes
Motivated by the desire to set a non-default formatter for all parsers including the top-level parser and subparsers.
Related issue(s)
Checklist
create_shared_parser
to fixaugur curate