Opened 4 years ago

Last modified 3 years ago

#7330 enhancement new

Add twisted.python.usage.argparseToOptions

Reported by: Julian Berman Owned by: Julian Berman
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/argparseToOptions-7330
branch-diff, diff-cov, branch-cov, buildbot
Author: julian

Description

It would be nice if usage provided a way of taking an argparse.ArgumentParser and using it in places that expect a usage.Options, allowing people who might be more familiar with the standard library module to still hook into places that interface with twisted's option parsing.

Change History (17)

comment:1 Changed 4 years ago by julian

Author: julian
Branch: branches/argparseToOptions-7330

(In [42632]) Branching to argparseToOptions-7330.

comment:2 Changed 4 years ago by Julian Berman

OK, this appears to be enough for me to at least be able to use an ArgumentParser with an Application. Would appreciate someone with some more knowledge of usage who might know if that's representative enough.

Build Results

comment:3 Changed 3 years ago by Julian Berman

Keywords: review added

I have no idea why I didn't put this up for review :/, but it's up now.

comment:4 Changed 3 years ago by Glyph

The build results link is dead, so I forced a new build.

comment:5 Changed 3 years ago by Jonathan Ballet

Keywords: review removed
Owner: set to Julian Berman

[disclaimer: I'm not a Twisted committer]

I had a look at the patch and I think it's overall a good idea. I appreciated the effort put into the documentation and interface specification.

As far as I read, the new interface you proposed is minimal but sufficient enough for the use case covered by the argument parser.

  • The buildbots were all green at the time of the review, except for the documentation one (see below).
  • There are no tests for argparseToOptions, beside the check for the interface specification, which is not sufficient.
  • In IArgumentParser, you should also document the result returned by each methods of the interface.
  • There are a few typos:

comment:6 Changed 3 years ago by Julian Berman

Keywords: review added
Owner: Julian Berman deleted

Hey, thanks for the review!

Added the direct tests, and fixed the typos. Putting back up for review.

comment:7 in reply to:  6 Changed 3 years ago by Jonathan Ballet

Replying to Julian:

Hey, thanks for the review!

Added the direct tests, and fixed the typos. Putting back up for review.

Hi Julian,

can you schedule a new build on the Buildbot, as far as I know, nothing has been triggered by your changes.

comment:8 Changed 3 years ago by Julian Berman

Yup, sorry, was trying to figure out what was making git-svn confused. Triggered a build now.

comment:9 Changed 3 years ago by Jonathan Ballet

Keywords: review removed
Owner: set to Julian Berman

Hi Julian,

thanks for the changes!

Most Buildbots are green, those which are red have a problem with Bazaar unable to get the branch (?), I'm going to ignore them for this review, but you might want to have a look into it.

I had a look at the tests, and you should add a failing test: I think the interface actually lacks a specification in case of error, since argparse.parse() actually raises a SystemExit error whereas Twisted's Options raises a usage.UsageError. You should probably add this in the documentation of IArgumentParser and wrap the argparse.parse() call to raise the right exception.

Beside this, it looks good to me.

comment:11 in reply to:  10 ; Changed 3 years ago by Jonathan Ballet

Replying to glyph:

Looks like the documentation builder spotted a typo: https://buildbot.twistedmatrix.com/builders/documentation/builds/4956/steps/api-documentation/logs/new%20pydoctor%20errors

That was from a previous build, Julian addressed this issue in [43548] already.

comment:12 in reply to:  11 ; Changed 3 years ago by Glyph

Replying to multani:

Replying to glyph:

Looks like the documentation builder spotted a typo: https://buildbot.twistedmatrix.com/builders/documentation/builds/4956/steps/api-documentation/logs/new%20pydoctor%20errors

That was from a previous build, Julian addressed this issue in [43548] already.

Hm. It's on the most recent build visible on buildbot. I've forced another, hopefully that will help.

comment:13 in reply to:  12 Changed 3 years ago by Jonathan Ballet

Replying to glyph:

Hm. It's on the most recent build visible on buildbot. I've forced another, hopefully that will help.

Hum, weird, this one was green yesterday, the one you triggered also is.

There are 2 red ones though (py-select-gc and twistedcheckers), but the first one has a test failure in twisted.internet.test.test_tcp and the other one shows new checkers errors in Manhole apparently (?), so I guess there are unrelated to this branch.

comment:14 in reply to:  9 ; Changed 3 years ago by Julian Berman

Keywords: review added

Replying to multani:

Hi Julian,

thanks for the changes!

Most Buildbots are green, those which are red have a problem with Bazaar unable to get the branch (?), I'm going to ignore them for this review, but you might want to have a look into it.

I had a look at the tests, and you should add a failing test: I think the interface actually lacks a specification in case of error, since argparse.parse() actually raises a SystemExit error whereas Twisted's Options raises a usage.UsageError. You should probably add this in the documentation of IArgumentParser and wrap the argparse.parse() call to raise the right exception.

Beside this, it looks good to me.

Yup, another great point. Added, and forced another build.

comment:15 Changed 3 years ago by Julian Berman

Owner: Julian Berman deleted

comment:16 Changed 3 years ago by Julian Berman

The new twisted checker errors build output seems unrelated, but I'm confused as to what it's even showing there.

comment:17 in reply to:  14 Changed 3 years ago by Jonathan Ballet

Keywords: review removed
Owner: set to Julian Berman

Replying to Julian:

Replying to multani:

I had a look at the tests, and you should add a failing test: I think the interface actually lacks a specification in case of error, since argparse.parse() actually raises a SystemExit error whereas Twisted's Options raises a usage.UsageError. You should probably add this in the documentation of IArgumentParser and wrap the argparse.parse() call to raise the right exception.

Beside this, it looks good to me.

Yup, another great point. Added, and forced another build.

I think checking for unrecognized arguments is necessary, but not sufficient; there are still some edge cases where SystemExit is raised, such as this one:

parser = argparse.ArgumentParser()
parser.add_argument("--foo")
options = usage.argparseToOptions(parser)()
options.parseOptions(["--foo"])

Which gives:

error: argument --foo: expected one argument

I'm not sure exactly what would be the best thing to do. Catching SystemExit is not sufficient since it doesn't carry on why parsing failed. On the other hand, the ArgumentParser.exit() or ArgumentParser.error() methods look like to be the best places to catch errors, but there's no hooking place for these methods, beside sub-classing ArgumentParser or monkey-patching them... So I'm not sure what to recommend in this case.

Note: See TracTickets for help on using tickets.