Skip to content
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

bpo-21150: add quick link/summary table to the top of argparse documentation #12005

Merged
merged 8 commits into from
Apr 18, 2022

Conversation

suhearsawho
Copy link
Contributor

@suhearsawho suhearsawho commented Feb 24, 2019

A few summary/quick links tables were added to the argparse module documentation in hopes of helping the user to better understand the module. These changes were based on the ones made by Louie Lu in the past, with a couple of revisions.

https://bugs.python.org/issue21150

@lisroach lisroach requested a review from rhettinger February 24, 2019 23:07
Type Example
====================== ===========================
Positional ``'foo'``
Optional ``'-v'``, ``'--verbose'``
Copy link
Member

@merwok merwok Feb 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an argparse term to name this kind of parameter?

Required vs. optional is orthogonal to arg vs. --param arg.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the argparse documentation, it seems like they use positional and optional for these parameters. Is there something that I can do to make this section more clear?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah if that’s the terminology then keep it.

… removed backtick marks around source links to maintain consistency throughout file
:class:`ArgumentParser` through the following::

>>> parser = argparse.ArgumentParser(prog='PROG', description='DESC',
... formatter_class=argparse.RawDescriptionHelpFormatter)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is too long (spawns an horizontal scrollbar in the example box, in the rendered HTML), can you please use hanging indentation, like:

parser = argparse.ArgumentParser(
    prog="PROG",
    description="DESC",
    formatter_class=argparse.RawDescriptionHelpFormatter,
)

and help_. An example of the function :func:`ArgumentParser.add_argument`
is as follows::

>>> parser.add_argument('-v', '--verbose', action='store_true',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency please also use hanging indentation here, with a single argument per line.

@bedevere-bot
Copy link

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

The :mod:`argparse` module's support for command-line interfaces is built
from the following:

The :class:`argparse.ArgumentParser` creates a new :class:`ArgumentParser`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion, as they points to the exact same place, it may be lighter by removing this 2nd link. Also it's obvious a class creates an instance of itself.

Suggested change
The :class:`argparse.ArgumentParser` creates a new :class:`ArgumentParser`
The :class:`argparse.ArgumentParser` creates a new parser


The :class:`argparse.ArgumentParser` creates a new :class:`ArgumentParser`
object. Commonly used arguments include prog_, description_, and
formatter_class_. For example, the user can create an instance of
Copy link
Member

@JulienPalard JulienPalard Sep 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the user often mention the person using the Python program, not the person writing it. Maybe just remove this sentence and remplace by a simple "Example::" ?

Yes I also try to shorten your paragraph a bit: it's just an introduction, the real thing is documented further down, so if you want for it to be read you have to keep it short, people tend to just skip the introductions.

The :func:`ArgumentParser.add_argument` is a function that is used
to define how a single command-line argument should be parsed. Commonly used
arguments include `name or flags`_, action_, default_, type_, required_,
and help_. An example of the function :func:`ArgumentParser.add_argument`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, you could drop the last sentence, its single link is redundent, make the paragraph a bit long, and add no real information, "Example::" is enough.

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


**Name or Flags Type**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This table links to nothing, which were the initial idea of this PR (adding table with links), so I bet it would read better as a sentence like The first parameter of add_argument tells if it's we're describing a positional argument (without dashes, like ``"foo"``) or an optional one (prefixed by dashes, likes `"--verbose"`). (I'm not native english, please reword as appropriate).

@rhettinger rhettinger closed this Apr 18, 2022
@rhettinger rhettinger reopened this Apr 18, 2022
@rhettinger
Copy link
Contributor

Closing and reopening to trigger CI checks.

@DanielNoord
Copy link
Contributor

@rhettinger This should have probably closed #65349 😄

cmaloney added a commit to cmaloney/cpython that referenced this pull request Feb 5, 2025
This relies on `bytearray.resize()` and `os.readinto()` to reduce copies
and match behavior of `_io.FileIO.readall()`.

There is still an extra copy, and thus twice the memory required,
compared to FileIO because there isn't a zero-copy  path from
`bytearray` -> `bytes` currently.
cmaloney added a commit to cmaloney/cpython that referenced this pull request Feb 5, 2025
Utilize `bytearray.resize()` and `os.readinto()` to reduce copies
and match behavior of `_io.FileIO.readall()`.

There is still an extra copy which means twice the memory required
compared to FileIO because there isn't a zero-copy  path from
`bytearray` -> `bytes` currently.

On my system reading a 2GB file
`./python -m test -M8g -uall test_largefile -m test.test_largefile.PyLargeFileTest.test_large_read -v`

Goes from ~2.7 seconds -> ~2.2 seconds
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants