refactor: switch from optparse to argparse #38
Conversation
| @@ -5,3 +5,7 @@ | |||
| """ | |||
|
|
|||
| from .cogapp import Cog as Cog, CogUsageError as CogUsageError, main as main | |||
|
|
|||
|
|
|||
| if __name__ == "__main__": | |||
There was a problem hiding this comment.
This was here to enable calling the app as python -m cogapp <cli_args>. https://docs.python.org/3/library/__main__.html#idiomatic-usage
I used it while testing out the --help output and figured it would be useful to keep. I'm fine taking it out if you don't want it there though.
| @@ -801,6 +592,8 @@ def main(self, argv): | |||
| except CogError as err: | |||
| self.prerr(err) | |||
| return 1 | |||
| except SystemExit as exit: | |||
cogapp/options.py
Outdated
|
|
||
|
|
||
| if sys.version_info >= (3, 8): | ||
| from argparse import _ExtendAction |
There was a problem hiding this comment.
This seems odd to have to import or duplicate a private class from argparse. Isn't there a supported way to do what we need?
There was a problem hiding this comment.
The supported way to do it is by passing the string "extend", but that's only available starting in Python 3.8. I'll refactor to use the string in that case though.
cogapp/options.py
Outdated
| width=HELP_WIDTH, | ||
| ) | ||
|
|
||
| def _split_lines(self, text, width): |
There was a problem hiding this comment.
Again we seem to be reaching into internal details. What is the advantage of using a supported module if we have to use unsupported interfaces in it?
There was a problem hiding this comment.
We don't have to necessarily. I wasn't thinking of it as problematic but you make a good point. It was purely for aesthetic reasons to add the blank line between option descriptions (which becomes more aesthetically pleasing once there are long options, e.g. see https://github.com/vergenzt/cog/blob/bd3cb2435419906c90ddd08febe8a7df7544260a/docs/running.rst).
I'll remove it though - it's probably not worth the hassle.
| getattr(ns, self.dest).update([arg]) | ||
|
|
||
|
|
||
| class Markers(NamedTuple): |
There was a problem hiding this comment.
I'd rather avoid NamedTuple if we can, won't a dataclass work?
| @@ -1046,7 +1053,7 @@ def test_print_output(self): | |||
| make_files(d) | |||
| self.cog.callable_main(["argv0", "-rP", "test.cog"]) | |||
| self.assertFilesSame("test.cog", "test.out") | |||
| output = self.output.getvalue() | |||
| output, errput = self.capsys.readouterr() | |||
There was a problem hiding this comment.
throughout: if we aren't using errput, we should assign output, _ = ..., or we should make assertions about errput.
| _parser: ClassVar = CogArgumentParser( | ||
| prog="cog", | ||
| usage=argparse.SUPPRESS, | ||
| description=rewrap(description), |
There was a problem hiding this comment.
What happens if we don't use rewrap?
vergenzt
force-pushed
the
gh-32-refactor-optparse-to-argparse
branch
from
c4e774c
to
9db093e
Compare
|
Sorry gtg for now - will reply to remainder ASAP |
To consolidate options definitions into a single place, and use the more modern Python option parsing library.