Skip to content

GH-124478: Cleanup argparse documentation #124877

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

Merged
merged 24 commits into from
Oct 8, 2024

Conversation

savannahostrowski
Copy link
Member

@savannahostrowski savannahostrowski commented Oct 2, 2024

This PR:

  1. Removes duplicative examples that are fairly basic and sufficiently covered in the tutorial
  2. Removes the quick reference tables, as removing duplicative examples moves the reference docs for ArgumentParser and add_argument much higher up the page. I don't think this is terribly necessary anymore.
  3. Move the upgrading to optparse guide to a separate page. Since optparse has been deprecated since 3.2, this is probably a minimally viewed part of the main argparse docs, so I think it's fair to have it on a separate page.
  4. Consolidates some examples
  5. Updates terminology (e.g., sub-command -> subcommand) for consistency.

Please let me know if there are other best practices we consistently use in other documentation areas but lack here. I'm happy to incorporate them here so we can get these documents feeling fresh!

(As a note, I tried to be somewhat conservative here as I do think many of the examples are 1) valuable for new users and 2) too advanced to add to the tutorial in its current form)

@bedevere-app bedevere-app bot added docs Documentation in the Doc dir skip news awaiting review labels Oct 2, 2024
@JelleZijlstra
Copy link
Member

I looked at the preview https://cpython-previews--124877.org.readthedocs.build/en/124877/library/argparse.html#module-argparse and skimmed the entire doc. Below are some things I noticed. None are new to this PR, so feel free to ignore them and I can file a separate PR with those cleanups.

  • It's distracting that the first few paragraphs link :mod:`argparse` 4 times, and it just links back to the top of the page. Would recommend suppressing those links with :mod:`!argparse`.
  • RawTextHelpFormatter description says "new lines"; I think it's normally spelled "newlines".
  • "ArgumentParser uses filesystem encoding and error handler to read the file containing arguments." this sentence reads oddly, maybe needs a "the"
  • "the action default will not over write it" -> I think "overwrite" is one word
  • I might remove the parser.add_argument('source_file', type=open) example. It doesn't make it easy to properly close the file, and as the text says later, FileNotFoundError would not be handled nicely.
  • FileType is formatted inconsistently in the paragraph that starts Even [FileType](https://cpython-previews--124877.org.readthedocs.build/en/124877/library/argparse.html#argparse.FileType) has its limitations for use with the type keyword.
  • The text after "class argparse.Action" should be indented by another level.
  • The docs for parse_known_intermixed_args are not properly indented.

@savannahostrowski
Copy link
Member Author

Thanks so much for such a thorough review, @JelleZijlstra. I think I can incorporate your feedback in this PR. I'll revisit this tomorrow evening.

Copy link
Contributor

@willingc willingc left a comment

Choose a reason for hiding this comment

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

Lovely work @savannahostrowski. Thank you!

@savannahostrowski
Copy link
Member Author

@JelleZijlstra, I applied all of your suggestions. I also found formatting inconsistencies in taking another pass related to indentation and style.

@JelleZijlstra JelleZijlstra added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Oct 8, 2024
@JelleZijlstra JelleZijlstra merged commit 37228bd into python:main Oct 8, 2024
25 checks passed
@miss-islington-app
Copy link

Thanks @savannahostrowski for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Sorry, @savannahostrowski and @JelleZijlstra, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 37228bd16e3ef97d32da08848552f7ef016d68ab 3.13

@miss-islington-app
Copy link

Sorry, @savannahostrowski and @JelleZijlstra, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 37228bd16e3ef97d32da08848552f7ef016d68ab 3.12

@bedevere-app
Copy link

bedevere-app bot commented Oct 8, 2024

GH-125162 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Oct 8, 2024
savannahostrowski added a commit to savannahostrowski/cpython that referenced this pull request Oct 8, 2024
(cherry picked from commit 37228bd)

Co-authored-by: Savannah Ostrowski <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
Co-authored-by: Tomas R <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Oct 8, 2024

GH-125164 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Oct 8, 2024
JelleZijlstra added a commit that referenced this pull request Oct 8, 2024
(cherry picked from commit 37228bd)

Co-authored-by: Jelle Zijlstra <[email protected]>
Co-authored-by: Tomas R <[email protected]>
JelleZijlstra added a commit that referenced this pull request Oct 8, 2024
(cherry picked from commit 37228bd)

Co-authored-by: Jelle Zijlstra <[email protected]>
Co-authored-by: Tomas R <[email protected]>
efimov-mikhail pushed a commit to efimov-mikhail/cpython that referenced this pull request Oct 9, 2024
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 skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants