gh-91832: Add 'required' attr to argparse.Action repr #91841

AbhigyanBose · 2022-04-22T22:37:28Z

Adding 'required' to names in Lib.argparse.Action

gh-91832:
Added 'required' to the list names in Lib.argparse.Action.
Changed constant strings that test the Action object.

Automerge-Triggered-By: GH:merwok

fatihkabakk · 2022-04-22T23:21:38Z

We got the same error, here is my pr #91840.
It's weird that when patchcheck made, it deletes whitespaces, but when not it throws error.
I also mentioned that problem on my comment in #91840 .

AbhigyanBose · 2022-04-22T23:28:05Z

Thanks for pointing it out. I just realized the same.

Lib/test/test_argparse.py

merwok · 2022-04-25T14:57:58Z

Misc/NEWS.d/next/Library/2022-04-23-03-24-00.gh-issue-91832.TyLi65.rst

@@ -0,0 +1 @@
+Added 'required' to names list in Lib.argparse.Action.


Python stdlib doesn’t use a root package name, Lib is just a source directory.

Suggested change

Added 'required' to names list in Lib.argparse.Action.

Added 'required' to names list in argparse.Action.

Also mentioning parameters may be clearer than names

names is the name of the list in Action class that we are adding the element to. I should've put it in a code block.

How does this look ?
"Added an element required to the list names in argparse.Action._get_kwargs"

Same thing. My point is that it does not matter that a line in the code uses names, it is not meaninfgul on its own when read by people in the NEWS.

How does this sound then:

Added a parameter "required" to the list returned by _get_kwargs in argparse.Action.

The bug really is: required is missing from repr output
So this fix is doing: Add 'required' attr to argparse.Action repr
🙂

Ahhh..... Sorry, I'm still new here, and trying to get to know things. Thanks for taking the time to help me understand.

No worry! There is info in the devguide: https://devguide.python.org/pullrequest/

(for future PRs, please don’t force push, it makes reviews harder)

terryjreedy · 2022-04-25T16:38:58Z

As suggested by Ronald Oussoren on core mentorship, changing a line like ' x ' to ' x \n' should solve the patchcheck failure.

In a future issue, parser.format_help might be modified to not format with trailing whitespace. I doubt that it is really needed.

Sidenote: I am not sure what you did, but force-pushing is rarely needed when revising PRs and sometimes messes them up.

AbhigyanBose · 2022-04-25T16:58:29Z

@terryjreedy Got it. I just wasn't sure if I should change an unrelated test case's content in this particular PR. If that's what you and @merwok recommend I'll add in '\n' and then run make patchcheck before I submit.

I could also create a separate PR to add '\n' to that test case.

terryjreedy · 2022-04-25T17:41:41Z

A separate PR would be OK. Azure Pipelines passing is not required but its failure from patchcheck failing is a nuisance.

terryjreedy · 2022-04-25T20:25:43Z

I think Zachary Ware's suggestion of '\x20' might be even better. (How to fix argparse is not obvious. It uses %-formatting and I check all %s and no of format string I looked at had trailing spaces of format character that would produce such.)

merwok · 2022-04-25T20:53:53Z

This format spec does funky alignment:

cpython/Lib/argparse.py

Line 521 in 94d0853

action_header = '%*s%-*s ' % tup

AbhigyanBose · 2022-04-27T12:37:31Z

@merwok and @terryjreedy

I created a new issue for this. I also proposed the solution I thought to be the simplest there.

…ue-91832

AbhigyanBose · 2022-04-28T14:10:12Z

#91984 has been resolved, thus make patchcheck is successful. Is there any other change required in this PR ?

Misc/NEWS.d/next/Library/2022-04-23-03-24-00.gh-issue-91832.TyLi65.rst

merwok

Thanks for the PR!

Minor change in news entry then this is good to go.
Can you check if backports to 3.10 and 3.9 are needed? (if the required attribute is present in these versions too)

…Li65.rst Co-authored-by: Éric <[email protected]>

AbhigyanBose · 2022-04-28T14:21:42Z

Yeah. Seems like backports to 3.10 and 3.9 are necessary.(required is absent in both versions)

miss-islington · 2022-04-28T14:50:26Z

@AbhigyanBose: Status check is done, and it's a success ✅ .

miss-islington · 2022-04-28T14:50:31Z

Thanks @AbhigyanBose for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒⛏🤖

bedevere-bot · 2022-04-28T14:50:48Z

GH-92021 is a backport of this pull request to the 3.10 branch.

bedevere-bot · 2022-04-28T14:50:51Z

GH-92022 is a backport of this pull request to the 3.9 branch.

…-91841) GH- Adding 'required' to names in Lib.argparse.Action pythongh-91832: Added 'required' to the list `names` in `Lib.argparse.Action`. Changed constant strings that test the Action object. Automerge-Triggered-By: GH:merwok (cherry picked from commit 4ed3900) Co-authored-by: Abhigyan Bose <[email protected]>

GH- Adding 'required' to names in Lib.argparse.Action gh-91832: Added 'required' to the list `names` in `Lib.argparse.Action`. Changed constant strings that test the Action object. Automerge-Triggered-By: GH:merwok (cherry picked from commit 4ed3900) Co-authored-by: Abhigyan Bose <[email protected]>

…-91841) GH- Adding 'required' to names in Lib.argparse.Action pythongh-91832: Added 'required' to the list `names` in `Lib.argparse.Action`. Changed constant strings that test the Action object. Automerge-Triggered-By: GH:merwok (cherry picked from commit 4ed3900) Co-authored-by: Abhigyan Bose <[email protected]>

bedevere-bot added the awaiting review label Apr 22, 2022

AbhigyanBose commented Apr 23, 2022

View reviewed changes

Lib/test/test_argparse.py Outdated Show resolved Hide resolved

AbhigyanBose commented Apr 23, 2022

View reviewed changes

Lib/test/test_argparse.py Outdated Show resolved Hide resolved

AbhigyanBose commented Apr 23, 2022

View reviewed changes

Lib/test/test_argparse.py Outdated Show resolved Hide resolved

AbhigyanBose commented Apr 23, 2022

View reviewed changes

Lib/test/test_argparse.py Outdated Show resolved Hide resolved

merwok changed the title ~~gh-91832: Adding 'required' to names in Lib.argparse.Action~~ gh-91832: Adding 'required' to names in argparse.Action Apr 25, 2022

merwok reviewed Apr 25, 2022

View reviewed changes

AbhigyanBose added 3 commits April 25, 2022 21:00

Adding 'required' to names in Lib.argparse.Action

8e74437

Fixed accidental removal of trailing whitespaces.

1447401

Changed NEWS to better reflect code change.

94d0853

AbhigyanBose force-pushed the fix-issue-91832 branch from daf292a to 94d0853 Compare April 25, 2022 16:07

merwok changed the title ~~gh-91832: Adding 'required' to names in argparse.Action~~ gh-91832: Add 'required' attr to argparse.Action repr Apr 25, 2022

AlexWaygood added the stdlib Python modules in the Lib dir label Apr 26, 2022

AbhigyanBose mentioned this pull request Apr 27, 2022

test_argparse.py trailing spaces within test strings removed by make patchcheck if file is modified #91984

Closed

Merge branch 'main' of https://github.com/python/cpython into fix-iss…

ca77e29

…ue-91832

merwok mentioned this pull request Apr 28, 2022

gh-91832: Fix 'required' attribute not showing up in repr of argparse… #91840

Closed

merwok reviewed Apr 28, 2022

View reviewed changes

Misc/NEWS.d/next/Library/2022-04-23-03-24-00.gh-issue-91832.TyLi65.rst Outdated Show resolved Hide resolved

merwok reviewed Apr 28, 2022

View reviewed changes

Update Misc/NEWS.d/next/Library/2022-04-23-03-24-00.gh-issue-91832.Ty…

cd0e30e

…Li65.rst Co-authored-by: Éric <[email protected]>

merwok added the needs backport to 3.9 only security fixes label Apr 28, 2022

merwok added needs backport to 3.10 only security fixes 🤖 automerge labels Apr 28, 2022

merwok approved these changes Apr 28, 2022

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Apr 28, 2022

miss-islington merged commit 4ed3900 into python:main Apr 28, 2022

bedevere-bot removed awaiting merge needs backport to 3.10 only security fixes labels Apr 28, 2022

bedevere-bot removed the needs backport to 3.9 only security fixes label Apr 28, 2022

		@@ -0,0 +1 @@
		Added 'required' to names list in Lib.argparse.Action.

	Added 'required' to names list in Lib.argparse.Action.
	Added 'required' to names list in argparse.Action.

Uh oh!

gh-91832: Add 'required' attr to argparse.Action repr #91841

gh-91832: Add 'required' attr to argparse.Action repr #91841

Conversation

AbhigyanBose commented Apr 22, 2022 • edited by miss-islington Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Adding 'required' to names in Lib.argparse.Action

Uh oh!

fatihkabakk commented Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AbhigyanBose commented Apr 22, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

merwok Apr 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AbhigyanBose Apr 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

merwok Apr 25, 2022

Choose a reason for hiding this comment

Uh oh!

AbhigyanBose Apr 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

merwok Apr 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AbhigyanBose Apr 25, 2022

Choose a reason for hiding this comment

Uh oh!

merwok Apr 25, 2022

Choose a reason for hiding this comment

Uh oh!

terryjreedy commented Apr 25, 2022

Uh oh!

AbhigyanBose commented Apr 25, 2022

Uh oh!

terryjreedy commented Apr 25, 2022

Uh oh!

terryjreedy commented Apr 25, 2022

Uh oh!

merwok commented Apr 25, 2022

Uh oh!

AbhigyanBose commented Apr 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AbhigyanBose commented Apr 28, 2022

Uh oh!

Uh oh!

merwok left a comment

Choose a reason for hiding this comment

Uh oh!

AbhigyanBose commented Apr 28, 2022

Uh oh!

miss-islington commented Apr 28, 2022

Uh oh!

miss-islington commented Apr 28, 2022

Uh oh!

bedevere-bot commented Apr 28, 2022

Uh oh!

bedevere-bot commented Apr 28, 2022

Uh oh!

Uh oh!

AbhigyanBose commented Apr 22, 2022 •

edited by miss-islington

Loading

fatihkabakk commented Apr 22, 2022 •

edited

Loading

merwok Apr 25, 2022 •

edited

Loading

AbhigyanBose Apr 25, 2022 •

edited

Loading

AbhigyanBose Apr 25, 2022 •

edited

Loading

merwok Apr 25, 2022 •

edited

Loading

AbhigyanBose commented Apr 27, 2022 •

edited

Loading