argparse: Add support for custom argument types. #1064
Conversation
|
I'd suggest to make this as much CPython-compatible as possible. It has this particular behavior: https://docs.python.org/3/library/argparse.html#type
So in CPython the resulting behavior is this: |
5c007f4 to
13541ff
Compare
Thanks for spotting that! I've made it more CPython-compatible, at the expense of an increased footprint by 100 more bytes. The case you mentioned should be handled, along with the exception handling limitations of the custom type parser (ie. let anything that's not Right now the main differences between this and CPython should be the lack of type registration, and |
Yeah should be fine. Also because those would lead to an error immediately, whereas the type/default thingie is harder to discover and could lead to surprising results. |
| args = [dest] | ||
| arg_type = kwargs.get("type", None) | ||
| if arg_type is not None: | ||
| if not callable(arg_type): |
There was a problem hiding this comment.
Not sure but for code size perhaps this could be left out? It would become clear eventually from the moment the argument is used.
There was a problem hiding this comment.
I've made it follow CPython here, type kwarg checks are performed before arguments are actually parsed.
There's another issue, though. Letting the code fail when performing the argument check has the side effect that non-callable types raise a ValueErrorTypeError which, for CPython compatibility, is caught and reported as an _ArgError that in turn will terminate execution.
If you don't have typing stubs set up for one reason or another, I'd say this is the kind of error you may want to catch as early as possible.
python-stdlib/argparse/argparse.py
Outdated
| if arg_type is not None: | ||
| if not callable(arg_type): | ||
| raise ValueError("type is not callable") | ||
| if default is not None and not isinstance(default, str): |
There was a problem hiding this comment.
This is not correct: it skips conversion when an argument is passed. Test like
parser.add_argument("-x", type=CustomArgType(1), default=1)
parser.add_argument("-g", type=CustomArgType(1), default=1)
parser.add_argument("-i", type=CustomArgType(1), default="1")
args = parser.parse_args(["-x", "3"])
print(args)
assert args.x == 4
assert args.g == 1
assert args.i == 2
There was a problem hiding this comment.
Unless I'm interpreting the CPython docs wrong, in your test case fragment:
- "-x" has an explicit non-string default and an argument, hence must be let through as-is with no processing applied
- "-g" has an explicit non-string default and no argument, hence it must be set to the default value with no processing applied to it
- "-i" has an explicit string default and no argument, hence the default has to be processed with the custom type converter
So, if my interpretation is correct, args.x should be set to "3", args.g should be set to 1, and args.i should be set to 2, which is what dumping the parsed args reports.
I apologise if I'm missing the point here, but I don't see where the problem is :|
Edit:
CPython seems to agree with my interpretation. After you remove the duplicate definition for "-g" you get this:
sh-5.3$ python python-stdlib/argparse/test_argparse.py
type is not callable
args(a=None, b=None, c=None, d=None, e=None, f=None, g=1, h=2, x='3', i=2)
Traceback (most recent call last):
File "/home/agatti/src/micropython-lib/python-stdlib/argparse/test_argparse.py", line 115, in <module>
assert args.x == 4
^^^^^^^^^^^
AssertionErrorThere was a problem hiding this comment.
Is it possible your CPython picks up argparse.py from the test_argparse directory?
import argparse
parser = argparse.ArgumentParser()
parser.add_argument("-a", type=int)
parser.add_argument("-b", type=str)
parser.add_argument("-c", type=CustomArgType(1))
parser.add_argument("-x", type=CustomArgType(1), default=1)
parser.add_argument("-y", type=PassThrough, default=None)
parser.add_argument("-g", type=CustomArgType(1), default=1)
parser.add_argument("-i", type=CustomArgType(1), default="1")
args = parser.parse_args(["-x", "3"])
print(args)
assert args.x == 4
assert args.g == 1
assert args.i == 2
assert args.y is None
and not placed in the same directory as micropython-lib's argparse.py, on CPython 3.13.7 yields Namespace(a=None, b=None, c=None, x=4, y=None, g=1, i=2)
but indeed on MicroPython
args(a=None, b=None, c=None, x='3', y=None, g=1, i=2)
Traceback (most recent call last):
File "C:\temp\testargparse.py", line 104, in <module>
AssertionError:
There was a problem hiding this comment.
That was it, missed that bit. Thanks for sticking with this, it was indeed an issue on my end, code-wise.
I'd partially chalk it up to CPython's documentation being not really clear for that. It should probably read (emphasis mine) "If the type keyword is used with the default keyword, the type converter is only applied to the default value if the default is a string.".
Still, the test file's behaviour in isolation under CPython now matches MicroPython's when using its own argparse implementation. Hope this should be it :)
There was a problem hiding this comment.
If the type keyword is used with the default keyword, the type converter is only applied to the default value if the default is a string.
Yes that's it.
Hope this should be it :)
I think the tests cover everything.
13541ff to
6936a30
Compare
This commit adds support for optional custom argument type validation to
argparse.ArgumentParser, allowing for shorter argument validation code
for both simple builtins and complex types.
For example, assuming that a particular command line argument must be an
integer, using "parser.add_argument('-a', type=int)" will make sure that
any value passed to that argument that cannot be converted into an
integer will trigger an argument validation error.
Signed-off-by: Alessandro Gatti <a.gatti@frob.it>
6936a30 to
2859d02
Compare
This PR adds support for optional custom argument type validation to argparse.ArgumentParser, allowing for shorter argument validation code for both simple builtins and complex types, as supported by CPython.
CPython also provides the
argparse.FileTypeclass to simplify argument file/directory handling, but that was not added to the MicroPython module. Its behaviour is rather easy to manually replicate if needed anyway.This change increases the size of the
argparsemodule by 291 bytes, and that's mostly due to the extra error handling and related text strings. Probably those new strings can be shortened, but given that this module isn't usually included in production code, the size increase shouldn't hurt too much I guess.The unit tests unfortunately do not cover the case of a parameter failing validation, as the module will automatically issue a
sys.exitcall on error, after printing the relevant exception message. If there are cleaner ways to test that without hijackingsys.exit, let me know so I can update the tests accordingly.