Summary

Rewrite argument parsing in rb-site to use Python's argparse module.

Review Request #11369 — Created Jan. 12, 2021 and submitted Jan. 13, 2021, 2:08 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

rb-site has historically used the legacy optparse module, which had a
number of drawbacks:

Options were defined in a global pool, so they could only be defined
by a single command.
Options for one command were therefore "allowed" by a second command.
When running management commands special -- option was required to
separate rb-site's arguments from the management command's.
There wasn't a good way to define per-command help.

This change addresses all this by moving to argparse and redoing a bit
of our command handling. Now, each command is its own sub-parser with
its own help text and arguments. Collision is no longer possible, and
each command can be considered fully independent.

The -- argument is no longer needed when running management commands,
as argparse allows us to capture any remaining options and pass them
through.

However, argparse is not perfect, and we had to work around some
issues. For instance, multi-paragraph help text is not doable by
default, so we had to apply some custom logic (overriding "private"
functions that haven't changed since it was introduced) to format text
ourselves.

Another gotcha is that they recommend using nargs='+' to capture one
or more unknown arguments (for our management command), but it's
actually pretty incompatible with sub-parsers. Instead, we have to use
an undocumented option, nargs=argparse.PARSER, to get the desired
behavior. Despite being undocumented, it forms the basis of sub-parsers,
does exactly what we need, and is heavily referenced in bug reports as a
better (yet undocumented) option for this purpose.

While there are things left to be desired, this is a far more
future-proof way of performing argument parsing, and provides a
foundation for further improvements.

Testing Done

Unit tests pass on all supported versions of Python.

Manually ran each command with all the variations in configuration that
made sense (such as upgrade with/without a site directory or -all-sites,
and manage with all sorts of management commands and options).

Repeated those tests for Python 2.7, 3.6, and 3.9, to check for any
regressions in behavior across versions.

Commits

Summary	ID
Rewrite argument parsing in rb-site to use Python's argparse module. rb-site has historically used the legacy `optparse` module, which had a number of drawbacks: * Options were defined in a global pool, so they could only be defined by a single command. * Options for one command were therefore "allowed" by a second command. * When running management commands special `--` option was required to separate rb-site's arguments from the management command's. * There wasn't a good way to define per-command help. This change addresses all this by moving to `argparse` and redoing a bit of our command handling. Now, each command is its own sub-parser with its own help text and arguments. Collision is no longer possible, and each command can be considered fully independent. The `--` argument is no longer needed when running management commands, as `argparse` allows us to capture any remaining options and pass them through. However, `argparse` is not perfect, and we had to work around some issues. For instance, multi-paragraph help text is not doable by default, so we had to apply some custom logic (overriding "private" functions that haven't changed since it was introduced) to format text ourselves. Another gotcha is that they recommend using `nargs='+'` to capture one or more unknown arguments (for our management command), but it's actually pretty incompatible with sub-parsers. Instead, we have to use an undocumented option, `nargs=argparse.PARSER`, to get the desired behavior. Despite being undocumented, it forms the basis of sub-parsers, does exactly what we need, and is heavily referenced in bug reports as a better (yet undocumented) option for this purpose. While there are things left to be desired, this is a far more future-proof way of performing argument parsing, and provides a foundation for further improvements.	a4e558d624bf911b4d9fbe4a18364440dd2b77f6

Summary

Rewrite argument parsing in rb-site to use Python's argparse module.

rb-site has historically used the legacy `optparse` module, which had a number of drawbacks: * Options were defined in a global pool, so they could only be defined by a single command. * Options for one command were therefore "allowed" by a second command. * When running management commands special `--` option was required to separate rb-site's arguments from the management command's. * There wasn't a good way to define per-command help. This change addresses all this by moving to `argparse` and redoing a bit of our command handling. Now, each command is its own sub-parser with its own help text and arguments. Collision is no longer possible, and each command can be considered fully independent. The `--` argument is no longer needed when running management commands, as `argparse` allows us to capture any remaining options and pass them through. However, `argparse` is not perfect, and we had to work around some issues. For instance, multi-paragraph help text is not doable by default, so we had to apply some custom logic (overriding "private" functions that haven't changed since it was introduced) to format text ourselves. Another gotcha is that they recommend using `nargs='+'` to capture one or more unknown arguments (for our management command), but it's actually pretty incompatible with sub-parsers. Instead, we have to use an undocumented option, `nargs=argparse.PARSER`, to get the desired behavior. Despite being undocumented, it forms the basis of sub-parsers, does exactly what we need, and is heavily referenced in bug reports as a better (yet undocumented) option for this purpose. While there are things left to be desired, this is a far more future-proof way of performing argument parsing, and provides a foundation for further improvements.

a4e558d624bf911b4d9fbe4a18364440dd2b77f6

Issues

Description	From	Last Updated
F841 local variable 'old_width' is assigned to but never used	reviewbot	Jan. 12, 2021, 2:51 a.m.
F811 redefinition of unused 'test_with_help_as_command_option' from line 240	reviewbot	Jan. 12, 2021, 2:51 a.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/cmdline/rbsite.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F841 local variable 'old_width' is assigned to but never used
```
reviewboard/cmdline/tests/test_rbsite.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F811 redefinition of unused 'test_with_help_as_command_option' from line 240
```

Change Summary:


Added some missing wrap width setup for help text that got left out of the posted change.
Removed a duplicate test method.

Commits:

	Summary	ID
	Rewrite argument parsing in rb-site to use Python's argparse module. rb-site has historically used the legacy `optparse` module, which had a number of drawbacks: * Options were defined in a global pool, so they could only be defined by a single command. * Options for one command were therefore "allowed" by a second command. * When running management commands special `--` option was required to separate rb-site's arguments from the management command's. * There wasn't a good way to define per-command help. This change addresses all this by moving to `argparse` and redoing a bit of our command handling. Now, each command is its own sub-parser with its own help text and arguments. Collision is no longer possible, and each command can be considered fully independent. The `--` argument is no longer needed when running management commands, as `argparse` allows us to capture any remaining options and pass them through. However, `argparse` is not perfect, and we had to work around some issues. For instance, multi-paragraph help text is not doable by default, so we had to apply some custom logic (overriding "private" functions that haven't changed since it was introduced) to format text ourselves. Another gotcha is that they recommend using `nargs='+'` to capture one or more unknown arguments (for our management command), but it's actually pretty incompatible with sub-parsers. Instead, we have to use an undocumented option, `nargs=argparse.PARSER`, to get the desired behavior. Despite being undocumented, it forms the basis of sub-parsers, does exactly what we need, and is heavily referenced in bug reports as a better (yet undocumented) option for this purpose. While there are things left to be desired, this is a far more future-proof way of performing argument parsing, and provides a foundation for further improvements.	9d40d52357c8b4430a5bab62592075e74d25129e
	Rewrite argument parsing in rb-site to use Python's argparse module. rb-site has historically used the legacy `optparse` module, which had a number of drawbacks: * Options were defined in a global pool, so they could only be defined by a single command. * Options for one command were therefore "allowed" by a second command. * When running management commands special `--` option was required to separate rb-site's arguments from the management command's. * There wasn't a good way to define per-command help. This change addresses all this by moving to `argparse` and redoing a bit of our command handling. Now, each command is its own sub-parser with its own help text and arguments. Collision is no longer possible, and each command can be considered fully independent. The `--` argument is no longer needed when running management commands, as `argparse` allows us to capture any remaining options and pass them through. However, `argparse` is not perfect, and we had to work around some issues. For instance, multi-paragraph help text is not doable by default, so we had to apply some custom logic (overriding "private" functions that haven't changed since it was introduced) to format text ourselves. Another gotcha is that they recommend using `nargs='+'` to capture one or more unknown arguments (for our management command), but it's actually pretty incompatible with sub-parsers. Instead, we have to use an undocumented option, `nargs=argparse.PARSER`, to get the desired behavior. Despite being undocumented, it forms the basis of sub-parsers, does exactly what we need, and is heavily referenced in bug reports as a better (yet undocumented) option for this purpose. While there are things left to be desired, this is a far more future-proof way of performing argument parsing, and provides a foundation for further improvements.	a4e558d624bf911b4d9fbe4a18364440dd2b77f6

Diff:

Revision 2 (+1306 -286)

Show changes

	reviewboard/cmdline/rbsite.py
	reviewboard/cmdline/tests/test_rbsite.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (0e38ed7)