Organize subcommands #1002

joverlee521 · 2022-07-07T01:04:59Z

Description of proposed changes

While starting on the new augur curate command, I was thinking of how to organize its suite of subcommands.
This is my proof-of-concept, reorganizing the existing measurements command as an example.

Testing

Functional test for the measurements command passed locally.

codecov · 2022-07-07T01:07:30Z

Codecov Report

Attention: Patch coverage is 90.65934% with 17 lines in your changes missing coverage. Please review.

Project coverage is 59.64%. Comparing base (e1c44c8) to head (60ba754).
Report is 1308 commits behind head on master.

Files	Patch %	Lines
augur/measurements/concat.py	50.00%	13 Missing ⚠️
augur/argparse_.py	78.94%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1002      +/-   ##
==========================================
+ Coverage   59.24%   59.64%   +0.40%     
==========================================
  Files          50       53       +3     
  Lines        6267     6317      +50     
  Branches     1588     1586       -2     
==========================================
+ Hits         3713     3768      +55     
+ Misses       2292     2286       -6     
- Partials      262      263       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

augur/measurements.py

augur/measurements/__init__.py

tsibley

+1 for the general pattern of separate command modules with subpackages. A couple notes, both relatively minor and non-blocking.

augur/measurements/__init__.py

augur/__init__.py

Instead of just having each command to add their own arguments, allow them add their own subparser. This gives each command complete control of its own parser and their subparsers to customize them as needed. This allows each command to define its own command name instead of parsing the command name from the module name, which is important for the following commit to rename the `import` module. This commit also introduces the behavior to print a command's help when called if it does not have its own `run` function. Note that for a majority of the commands, `register_arguments` was updated to `register_parser`. However, for `align`, `filter` and `mask`, I kept the `register_arguments` functions since they are used in unit testing. This change came from discussion of how to extract the boilerplate for registering subparsers so that commands can use the same function to register their own subcommands.¹ ¹ #1002 (comment)

@tsibley

Since `import` is a Python keyword, the best practice is to name the module as `import_` according to PEP-8 guidelines.¹ This should not affect the usage of the `augur import` command since the command name remains unchanged. Since the module only has `register_parser` and `run` functions, there is unlikely to be any external uses of `augur.import`. This change came from suggestion by @tsibley in review.² ¹ https://peps.python.org/pep-0008/#descriptive-naming-styles ² #1002 (comment)

joverlee521 · 2022-07-12T18:17:01Z

Tests failed at the setup-miniconda step, so I manually re-ran the pull_request tests (all passed). Skipped the push tests since they are just a duplicate of the pull_request tests.

augur/__init__.py

joverlee521 · 2022-07-13T18:56:16Z

augur/argparse_.py

+
+        if not subparser.description and command.__doc__:
+            subparser.description = command.__doc__
+


Ideally, I would like to add the subparser's help message here instead of requiring each module to import utils.first_line. However, I have not found a way to add help outside of the add_parser call.

When I tried:

if not subparser.help and command.__doc__: subparser.help = first_line(command.__doc__)

I get an error AttributeError: 'ArgumentParser' object has no attribute 'help'

Ah, argparse uses private methods to add the help message behind the scenes.

Yeah. help= becomes a property of a choices action on the parser's subparsers object, not the subparser itself.

It's retrievable, e.g.

name = next(name for name, sp in subparsers.choices.items() if sp is subparser) choice = next(c for c in s._choices_actions if c.dest == name) choice.help = …

but may not want to do that here!

Hmm, seems a bit hacky to do this...

# Add help message for subparser if it doesn't already exist name = next(name for name, sp in subparsers.choices.items() if sp is subparser) if not any(c.dest == name for c in subparsers._choices_actions) and command.__doc__: aliases = [alias for alias, sp in subparsers._name_parser_map.items() if alias != name and sp is subparser] choice_action = subparsers._ChoicesPseudoAction(name, aliases, help=first_line(command.__doc__)) subparsers._choices_actions.append(choice_action)

Yeah, agreed!

augur/argparse_.py

@tsibley

This `argparse_` module is expected to house custom helpers for interacting/extending the `argparse` standard library. Move `add_default_command()` to a shared `argparse_` module so that it can be used by commands without having circular import issues. I named the module `argparse_` (with an underscore) to avoid overwriting the `argparse` standard library within `augur/__init__.py`. @tsibley found the relevant docs¹ to explain this behavior. ¹ https://docs.python.org/3/reference/import.html#submodules

Instead of just having each command to add their own arguments, allow them add their own subparser. This gives each command complete control of its own parser and their subparsers to customize them as needed. This allows each command to define its own command name instead of parsing the command name from the module name, which is important for the following commit to rename the `import` module. This commit also introduces the behavior to print a command's help when called if it does not have its own `run` function. Note that for a majority of the commands, `register_arguments` was updated to `register_parser`. However, for `align`, `filter` and `mask`, I kept the `register_arguments` functions since they are used in unit testing. This change came from discussion of how to extract the boilerplate for registering subparsers so that commands can use the same function to register their own subcommands.¹ ¹ #1002 (comment)

@tsibley

Since `import` is a Python keyword, the best practice is to name the module as `import_` according to PEP-8 guidelines.¹ This should not affect the usage of the `augur import` command since the command name remains unchanged. Since the module only has `register_parser` and `run` functions, there is unlikely to be any external uses of `augur.import`. This change came from suggestion by @tsibley in review.² ¹ https://peps.python.org/pep-0008/#descriptive-naming-styles ² #1002 (comment)

This is a custom argparse.Action so it belongs with the other argparse helpers. This was added with the augur measurements command so it's highly unlikely to be used externally.

Moves the measurements module to its own package where the subcommands are split out into their own modules. The subcommand subparsers are added with the new `add_command_subparsers` function. This is an effort to reorganize augur commands with subcommands as packages instead of continuously adding subcommands as modules. Starting with the measurements command since it is a new command that is unlikely to already be used by anyone outside of the Nextstrain. In the future, we may want to consider refactoring export and validate in the same way. Putting that off for now since reorganizing those modules may result in breaking outside uses of their APIs.

Moves the import_ and import_beast modules to an import_ package and uses the new `add_command_subparsers` function to add the beast subparser. Search via cs.github.com¹ shows that there are no external uses of the `augur.import` or `augur.import_beast` APIs, so this should be safe to move to a package. ¹ https://cs.github.com/?scopeName=All+repos&scope=&q=%22augur.import%22

Simplifies the `register_parser` function and removes the now unneeded `run` function in export.py Ideally, export and it subcommands would be moved to its own package, but keeping these files in place since moving them can potentially break external uses of their APIs.

victorlin

Much cleaner and understandable this way! Just a few minor suggestions.

augur/measurements/export.py

augur/distance.py

augur/export_v2.py

These were carried over from when the two command subparsers were created within the same parent parser. Now that they are separated, the argument group variables can be renamed to remove redundant words.

`utils.first_line` is useful for the brief help message for `augur/distance.py` since it has a long multiline docstring, but seems like an overkill for other modules. Keep things simple by removing `utils.first_line` from other modules and just use their docstrings directly as their help message.

Keep the export modules consistent with the other modules in Augur and create help messages from the module docstrings.

victorlin requested changes Jul 7, 2022

View reviewed changes

augur/measurements.py Outdated Show resolved Hide resolved

joverlee521 commented Jul 8, 2022

View reviewed changes

augur/measurements.py Outdated Show resolved Hide resolved

joverlee521 force-pushed the organize-subcommands branch from 9e1cce9 to 1b2d1e2 Compare July 8, 2022 21:23

joverlee521 marked this pull request as ready for review July 8, 2022 21:30

joverlee521 requested a review from a team July 8, 2022 21:30

victorlin requested changes Jul 9, 2022

View reviewed changes

augur/measurements/__init__.py Outdated Show resolved Hide resolved

tsibley self-requested a review July 11, 2022 17:01

tsibley reviewed Jul 11, 2022

View reviewed changes

augur/measurements/__init__.py Outdated Show resolved Hide resolved

augur/__init__.py Outdated Show resolved Hide resolved

joverlee521 force-pushed the organize-subcommands branch from 1b2d1e2 to 615a8ae Compare July 12, 2022 17:50

victorlin mentioned this pull request Jul 12, 2022

filter: Split filter.py into smaller files #997

Merged

joverlee521 commented Jul 13, 2022

View reviewed changes

augur/__init__.py Outdated Show resolved Hide resolved

tsibley mentioned this pull request Jul 13, 2022

dev: Avoid a confusing import binding situation nextstrain/cli#196

Merged

1 task

joverlee521 commented Jul 13, 2022

View reviewed changes

augur/argparse_.py Outdated Show resolved Hide resolved

joverlee521 added 7 commits July 14, 2022 11:23

Move HideAsFalseAction to argparse_ module

d531d48

This is a custom argparse.Action so it belongs with the other argparse helpers. This was added with the augur measurements command so it's highly unlikely to be used externally.

joverlee521 force-pushed the organize-subcommands branch from 615a8ae to 7de1fcd Compare July 14, 2022 18:50

victorlin approved these changes Jul 15, 2022

View reviewed changes

augur/measurements/export.py Outdated Show resolved Hide resolved

augur/distance.py Show resolved Hide resolved

augur/export_v2.py Outdated Show resolved Hide resolved

joverlee521 added 3 commits July 19, 2022 12:32

measurements: rename parser group variables

9b6107d

These were carried over from when the two command subparsers were created within the same parent parser. Now that they are separated, the argument group variables can be renamed to remove redundant words.

export: create help message from module docstrings

6df2066

Keep the export modules consistent with the other modules in Augur and create help messages from the module docstrings.

Update changelog

60ba754

victorlin approved these changes Jul 20, 2022

View reviewed changes

joverlee521 merged commit cea6b96 into master Jul 20, 2022

joverlee521 deleted the organize-subcommands branch July 20, 2022 17:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Organize subcommands #1002

Organize subcommands #1002

joverlee521 commented Jul 7, 2022

codecov bot commented Jul 7, 2022 •

edited

Loading

tsibley left a comment

joverlee521 commented Jul 12, 2022

joverlee521 Jul 13, 2022

joverlee521 Jul 13, 2022

tsibley Jul 13, 2022

joverlee521 Jul 13, 2022 •

edited

Loading

tsibley Jul 15, 2022

victorlin left a comment


		if not subparser.description and command.__doc__:
		subparser.description = command.__doc__

Organize subcommands #1002

Organize subcommands #1002

Conversation

joverlee521 commented Jul 7, 2022

Description of proposed changes

Testing

codecov bot commented Jul 7, 2022 • edited Loading

Codecov Report

tsibley left a comment

Choose a reason for hiding this comment

joverlee521 commented Jul 12, 2022

joverlee521 Jul 13, 2022

Choose a reason for hiding this comment

joverlee521 Jul 13, 2022

Choose a reason for hiding this comment

tsibley Jul 13, 2022

Choose a reason for hiding this comment

joverlee521 Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

tsibley Jul 15, 2022

Choose a reason for hiding this comment

victorlin left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 7, 2022 •

edited

Loading

joverlee521 Jul 13, 2022 •

edited

Loading