Summary

Add base classes for multi-commands.

Review Request #11882 — Created Nov. 24, 2021 and submitted March 7, 2022, 4:20 p.m.

Information

Owner

david

Repository

RBTools

Branch

master

Bugs

Depends On

Blocks

11883

Reviewers

Groups

rbtools

People

Description

We'll soon be adding two new commands, rbt api and rbt review, which
both want to implement additional subcommands (rbt api get,
rbt review publish, etc). This change adds the framework to do so,
consisting of two new base classes. SubCommand should be inherited for
each subcommand offered, and MultiCommand aggregates them together.
Arguments from the MultiCommand are added in to the subcommand
parsers.

The one tricky thing here was getting --help to do the right thing. In
the case of MultiCommands, we need to actually parse the arguments in
order to get the right subparser, but doing so was enforcing any usage
requirements. It turns out that the correct fix here was to allow
add_help to default to True in the parser initialization. This
allows us to call parse_args and have it not fail.

I did also have to move the logging setup out of the initialize
method, so that we wouldn't attach multiple log handlers.

Testing Done

Used this extensively with the upcoming review command.

Commits

Summary	ID
Add base classes for multi-commands. We'll soon be adding two new commands, `rbt api` and `rbt review`, which both want to implement additional subcommands (`rbt api get`, `rbt review publish`, etc). This change adds the framework to do so, consisting of two new base classes. `SubCommand` should be inherited for each subcommand offered, and `MultiCommand` aggregates them together. Arguments from the `MultiCommand` are added in to the subcommand parsers. The one tricky thing here was getting `--help` to do the right thing. In the case of `MultiCommand`s, we need to actually parse the arguments in order to get the right subparser, but doing so was enforcing any usage requirements. It turns out that the correct fix here was to allow `add_help` to default to `True` in the parser initialization. This allows us to call `parse_args` and have it not fail. I did also have to move the logging setup out of the `initialize` method, so that we wouldn't attach multiple log handlers. Testing Done: Used this extensively with the upcoming review command.	9c1d7311adb3f3f81f9fb99822dc2da918323c2e

Summary

Add base classes for multi-commands.

We'll soon be adding two new commands, `rbt api` and `rbt review`, which both want to implement additional subcommands (`rbt api get`, `rbt review publish`, etc). This change adds the framework to do so, consisting of two new base classes. `SubCommand` should be inherited for each subcommand offered, and `MultiCommand` aggregates them together. Arguments from the `MultiCommand` are added in to the subcommand parsers. The one tricky thing here was getting `--help` to do the right thing. In the case of `MultiCommand`s, we need to actually parse the arguments in order to get the right subparser, but doing so was enforcing any usage requirements. It turns out that the correct fix here was to allow `add_help` to default to `True` in the parser initialization. This allows us to call `parse_args` and have it not fail. I did also have to move the logging setup out of the `initialize` method, so that we wouldn't attach multiple log handlers. Testing Done: Used this extensively with the upcoming review command.

9c1d7311adb3f3f81f9fb99822dc2da918323c2e

Issues

Description	From	Last Updated
Can we modernize the docs while here?	chipx86	Dec. 3, 2021, 8:48 a.m.
Can we name this BaseSubCommand?	chipx86	Dec. 3, 2021, 9:14 a.m.
Is this a default help text, or turning off help text? Can we add a comment/doc comment?	chipx86	Dec. 3, 2021, 8:52 a.m.
This is missing a docstring.	chipx86	Dec. 3, 2021, 8:52 a.m.
Can we name this BaseMultiCommand?	chipx86	Dec. 3, 2021, 9:14 a.m.
Needs a double backtick, or it will be a reference. Or, ideally: :command:`rbt review `	chipx86	Dec. 3, 2021, 8:52 a.m.
, optional	chipx86	Dec. 3, 2021, 8:52 a.m.
What happens if this triggers an argument parsing error condition (like an invalid argument)? Those result in a sys.exit, which …	chipx86	Dec. 3, 2021, 9:14 a.m.
F821 undefined name 'SubCommand'	reviewbot	Dec. 3, 2021, 10:04 a.m.
F821 undefined name 'MultiCommand'	reviewbot	Dec. 3, 2021, 10:04 a.m.
"review groups" or "option groups"?	chipx86	March 7, 2022, 9:21 a.m.
Missing Args:, args, and *kwargs.	chipx86	March 7, 2022, 9:22 a.m.
Should be in sorted alphabetical order (capitalized is first).	chipx86	March 7, 2022, 9:23 a.m.

flake8 passed.

JSHint passed.

rbtools/commands/__init__.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Can we modernize the docs while here?
```

rbtools/commands/__init__.py (Diff revision 1)

The issue has been resolved. Show all issues

Is this a default help text, or turning off help text? Can we add a comment/doc comment?

rbtools/commands/__init__.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This is missing a docstring.
```

rbtools/commands/__init__.py (Diff revision 1)

The issue has been resolved. Show all issues

Needs a double backtick, or it will be a reference. Or, ideally: :command:`rbt review `

rbtools/commands/__init__.py (Diff revision 1)
The issue has been resolved. Show all issues
```
, optional
```

rbtools/commands/main.py (Diff revision 1)

The issue has been dropped. Show all issues

What happens if this triggers an argument parsing error condition (like an invalid argument)? Those result in a sys.exit, which we'd have to catch (SystemExit). We'd probably have to catch ArgumentError as well.

david Dec. 3, 2021, 9:14 a.m.

This is handled correctly:

$ rbt review ax
usage: rbt review  <subcommand> [options]

Creates, updates, and edits reviews.
rbt review: error: invalid choice: 'ax' (choose from 'edit', 'publish', 'add-diff-comment', 'add-general-comment', 'add-file-attachment-comment', 'discard')

$ rbt review edit -r 1 --blah
usage: rbt review  <subcommand> [options]

Creates, updates, and edits reviews.
rbt review: error: unrecognized arguments: --blah

rbtools/commands/__init__.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Can we name this BaseSubCommand?
```
rbtools/commands/__init__.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Can we name this BaseMultiCommand?
```

Commits:

	Summary	ID
	Add base classes for multi-commands. We'll soon be adding two new commands, `rbt api` and `rbt review`, which both want to implement additional subcommands (`rbt api get`, `rbt review publish`, etc). This change adds the framework to do so, consisting of two new base classes. `SubCommand` should be inherited for each subcommand offered, and `MultiCommand` aggregates them together. Arguments from the `MultiCommand` are added in to the subcommand parsers. The one tricky thing here was getting `--help` to do the right thing. In the case of `MultiCommand`s, we need to actually parse the arguments in order to get the right subparser, but doing so was enforcing any usage requirements. It turns out that the correct fix here was to allow `add_help` to default to `True` in the parser initialization. This allows us to call `parse_args` and have it not fail. I did also have to move the logging setup out of the `initialize` method, so that we wouldn't attach multiple log handlers. Testing Done: Used this extensively with the upcoming review command.	6180547375f99384d56926c0a451d843a6f5c66f
	Add base classes for multi-commands. We'll soon be adding two new commands, `rbt api` and `rbt review`, which both want to implement additional subcommands (`rbt api get`, `rbt review publish`, etc). This change adds the framework to do so, consisting of two new base classes. `SubCommand` should be inherited for each subcommand offered, and `MultiCommand` aggregates them together. Arguments from the `MultiCommand` are added in to the subcommand parsers. The one tricky thing here was getting `--help` to do the right thing. In the case of `MultiCommand`s, we need to actually parse the arguments in order to get the right subparser, but doing so was enforcing any usage requirements. It turns out that the correct fix here was to allow `add_help` to default to `True` in the parser initialization. This allows us to call `parse_args` and have it not fail. I did also have to move the logging setup out of the `initialize` method, so that we wouldn't attach multiple log handlers. Testing Done: Used this extensively with the upcoming review command.	18a029ec319184099954b19bb70f435decd7cefc

Diff:

Revision 2 (+368 -20)

Show changes

	rbtools/commands/__init__.py
	rbtools/commands/main.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/commands/__init__.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F821 undefined name 'SubCommand'
```
rbtools/commands/__init__.py (Diff revision 2)
The issue has been resolved. Show all issues
```
F821 undefined name 'MultiCommand'
```

Commits:

	Summary	ID
	Add base classes for multi-commands. We'll soon be adding two new commands, `rbt api` and `rbt review`, which both want to implement additional subcommands (`rbt api get`, `rbt review publish`, etc). This change adds the framework to do so, consisting of two new base classes. `SubCommand` should be inherited for each subcommand offered, and `MultiCommand` aggregates them together. Arguments from the `MultiCommand` are added in to the subcommand parsers. The one tricky thing here was getting `--help` to do the right thing. In the case of `MultiCommand`s, we need to actually parse the arguments in order to get the right subparser, but doing so was enforcing any usage requirements. It turns out that the correct fix here was to allow `add_help` to default to `True` in the parser initialization. This allows us to call `parse_args` and have it not fail. I did also have to move the logging setup out of the `initialize` method, so that we wouldn't attach multiple log handlers. Testing Done: Used this extensively with the upcoming review command.	18a029ec319184099954b19bb70f435decd7cefc
	Add base classes for multi-commands. We'll soon be adding two new commands, `rbt api` and `rbt review`, which both want to implement additional subcommands (`rbt api get`, `rbt review publish`, etc). This change adds the framework to do so, consisting of two new base classes. `SubCommand` should be inherited for each subcommand offered, and `MultiCommand` aggregates them together. Arguments from the `MultiCommand` are added in to the subcommand parsers. The one tricky thing here was getting `--help` to do the right thing. In the case of `MultiCommand`s, we need to actually parse the arguments in order to get the right subparser, but doing so was enforcing any usage requirements. It turns out that the correct fix here was to allow `add_help` to default to `True` in the parser initialization. This allows us to call `parse_args` and have it not fail. I did also have to move the logging setup out of the `initialize` method, so that we wouldn't attach multiple log handlers. Testing Done: Used this extensively with the upcoming review command.	1b235b2d7d82590fa3b80f309ab4dff59df1408a

Diff:

Revision 3 (+370 -20)

Show changes

	rbtools/commands/__init__.py
	rbtools/commands/main.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/commands/__init__.py (Diff revision 3)
The issue has been resolved. Show all issues
```
"review groups" or "option groups"?
```

rbtools/commands/__init__.py (Diff revision 3)

The issue has been resolved. Show all issues

Missing Args:, *args, and **kwargs.

rbtools/commands/main.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Should be in sorted alphabetical order (capitalized is first).
```

Commits:

	Summary	ID
	Add base classes for multi-commands. We'll soon be adding two new commands, `rbt api` and `rbt review`, which both want to implement additional subcommands (`rbt api get`, `rbt review publish`, etc). This change adds the framework to do so, consisting of two new base classes. `SubCommand` should be inherited for each subcommand offered, and `MultiCommand` aggregates them together. Arguments from the `MultiCommand` are added in to the subcommand parsers. The one tricky thing here was getting `--help` to do the right thing. In the case of `MultiCommand`s, we need to actually parse the arguments in order to get the right subparser, but doing so was enforcing any usage requirements. It turns out that the correct fix here was to allow `add_help` to default to `True` in the parser initialization. This allows us to call `parse_args` and have it not fail. I did also have to move the logging setup out of the `initialize` method, so that we wouldn't attach multiple log handlers. Testing Done: Used this extensively with the upcoming review command.	1b235b2d7d82590fa3b80f309ab4dff59df1408a
	Add base classes for multi-commands. We'll soon be adding two new commands, `rbt api` and `rbt review`, which both want to implement additional subcommands (`rbt api get`, `rbt review publish`, etc). This change adds the framework to do so, consisting of two new base classes. `SubCommand` should be inherited for each subcommand offered, and `MultiCommand` aggregates them together. Arguments from the `MultiCommand` are added in to the subcommand parsers. The one tricky thing here was getting `--help` to do the right thing. In the case of `MultiCommand`s, we need to actually parse the arguments in order to get the right subparser, but doing so was enforcing any usage requirements. It turns out that the correct fix here was to allow `add_help` to default to `True` in the parser initialization. This allows us to call `parse_args` and have it not fail. I did also have to move the logging setup out of the `initialize` method, so that we wouldn't attach multiple log handlers. Testing Done: Used this extensively with the upcoming review command.	9c1d7311adb3f3f81f9fb99822dc2da918323c2e

Diff:

Revision 4 (+384 -20)

Show changes

	rbtools/commands/__init__.py
	rbtools/commands/main.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to master (c3c2178)