-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
bpo-21150: Add summary of argparse #1210
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@lulouie, thanks for your PR! By analyzing the history of the files in this pull request, we identified @benjaminp, @birkenfeld and @ezio-melotti to be potential reviewers. |
^^^^^^^^^^^^^ | ||
|
||
.. csv-table:: | ||
:header: "Name", "Common Used Arguments", "Example" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commonly*
|
||
":class:`argparse.ArgumentParser`", "`prog`_, `description`_, `formatter_class`_", "``parser = argparse.ArgumentParser(prog='PROG', description='DESC', formatter_class=argparse.RawDescriptionHelpFormatter)``" | ||
":func:`ArgumentParser.add_argument`", "`name or flags`_, `action`_, `default`_, `type`_, `required`_, `help`_", "``parser.add_argument('-v', '--verbose', action='store_true', help='Show various debugging information')``" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not have two small code blocks rather than a table here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With a code block it is not possible to link to the individual args, but I'm not sure that's really necessary since there are already links in one of the following tables.
@@ -185,6 +240,8 @@ ArgumentParser objects | |||
The following sections describe how each of these are used. | |||
|
|||
|
|||
.. _prog: | |||
|
|||
prog | |||
^^^^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the target markup is not necessary. The headings are automatically targets, but the markup to create the link is different (if I remember right):
`:ref:`prog`
`const`_ The constant values of name or flags | ||
`choices`_ Arguments should selected from this set or values ``['foo', 'bar']``, ``range(1, 10)``, Any object that support ``in`` operator | ||
`required`_ Indicate a optional argument is required or not ``True``, ``False`` | ||
`metavar`_ A alternative display name for argument |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/A/An/
====================== ================================================== ======================================================================================================================= | ||
Name Description Keywords | ||
====================== ================================================== ======================================================================================================================= | ||
`nargs`_ Associates number of arguments ``N`` (an integer), ``'?'``, ``'*'``, ``'+'``, ``argparse.REMAINDER`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This description is not particularly clear.
You should also decide if you want to use 2nd or 3rd person and do it consistently (e.g. this is 3rd, but then you have "indicate" which is 2nd/imperative).
You could use :class:`int`
instead of integer.
`nargs`_ Associates number of arguments ``N`` (an integer), ``'?'``, ``'*'``, ``'+'``, ``argparse.REMAINDER`` | ||
`const`_ The constant values of name or flags | ||
`choices`_ Arguments should selected from this set or values ``['foo', 'bar']``, ``range(1, 10)``, Any object that support ``in`` operator | ||
`required`_ Indicate a optional argument is required or not ``True``, ``False`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/a /an /
====================== =============================================== ========================================================================================================================= | ||
Name Description Keywords | ||
====================== =============================================== ========================================================================================================================= | ||
`action`_ What to do on arguments ``'store'``, ``'store_const'``, ``'store_true'``, ``'append'``, ``append_const``, ``count``, ``help``, ``version`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe s/on/with the/?
The last few values are missing the quotes.
`action`_ What to do on arguments ``'store'``, ``'store_const'``, ``'store_true'``, ``'append'``, ``append_const``, ``count``, ``help``, ``version`` | ||
`default`_ Default value of arguments | ||
`type`_ Treat arguments as type ``int``, ``float``, ``bool``, ``argparse.FileType('w')``, ``callable function`` | ||
`help`_ Help message of arguments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use singular "argument" throughout the table, since add_argument and its options only affect a single argument.
Summary | ||
------- | ||
|
||
Core function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/function/functions/ ?
Also ArgumentParser
is a class, not a function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe «Core functionality» was meant.
|
||
":class:`argparse.ArgumentParser`", "`prog`_, `description`_, `formatter_class`_", "``parser = argparse.ArgumentParser(prog='PROG', description='DESC', formatter_class=argparse.RawDescriptionHelpFormatter)``" | ||
":func:`ArgumentParser.add_argument`", "`name or flags`_, `action`_, `default`_, `type`_, `required`_, `help`_", "``parser.add_argument('-v', '--verbose', action='store_true', help='Show various debugging information')``" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With a code block it is not possible to link to the individual args, but I'm not sure that's really necessary since there are already links in one of the following tables.
====================== =============================================== ========================================================================================================================= | ||
`action`_ What to do on arguments ``'store'``, ``'store_const'``, ``'store_true'``, ``'append'``, ``append_const``, ``count``, ``help``, ``version`` | ||
`default`_ Default value of arguments | ||
`type`_ Treat arguments as type ``int``, ``float``, ``bool``, ``argparse.FileType('w')``, ``callable function`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Automatically converts the argument to the given type"
Name Description Keywords | ||
====================== ================================================== ======================================================================================================================= | ||
`nargs`_ Associates number of arguments ``N`` (an integer), ``'?'``, ``'*'``, ``'+'``, ``argparse.REMAINDER`` | ||
`const`_ The constant values of name or flags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also not particularly clear.
====================== ================================================== ======================================================================================================================= | ||
`nargs`_ Associates number of arguments ``N`` (an integer), ``'?'``, ``'*'``, ``'+'``, ``argparse.REMAINDER`` | ||
`const`_ The constant values of name or flags | ||
`choices`_ Arguments should selected from this set or values ``['foo', 'bar']``, ``range(1, 10)``, Any object that support ``in`` operator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"A container that lists the possible values"
(container could link to the glossary entry if there is one)
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
@merwok @ezio-melotti What's the status on this? |
Review comments haven’t been addressed |
I think the current state of the patch is hopeless. If some one is inclined to take a more professional pass at this, I think they would be better off starting from scratch. |
No description provided.