Movatterモバイル変換

Lib/test/test_argparse.py OutdatedShow resolvedHide resolved

AbhigyanBose commented

Lib/test/test_argparse.py OutdatedShow resolvedHide resolved

AbhigyanBose commented

Lib/test/test_argparse.py OutdatedShow resolvedHide resolved

AbhigyanBose commented

Lib/test/test_argparse.py OutdatedShow resolvedHide 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

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

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

Copy link

Member

merwokApr 25, 2022•
edited
Loading

Choose a reason for hiding this comment

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

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

Suggested change

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

Also mentioningparameters may be clearer thannames

Copy link

ContributorAuthor

AbhigyanBoseApr 25, 2022•
edited
Loading

Choose a reason for hiding this comment

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

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 elementrequired to the listnames in argparse.Action._get_kwargs"

Copy link

Member

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

AbhigyanBoseApr 25, 2022•
edited
Loading

Choose a reason for hiding this comment

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

How does this sound then:

Added a parameter "required" to the list returned by_get_kwargs inargparse.Action.

Copy link

Member

merwokApr 25, 2022•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

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

Copy link

Member

Choose a reason for hiding this comment

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

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)

AbhigyanBose added3 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 thefix-issue-91832 branch fromdaf292a to94d0853Compare

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

Copy link

Member

terryjreedy commentedApr 25, 2022

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.

Copy link

ContributorAuthor

AbhigyanBose commentedApr 25, 2022

@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 runmake patchcheck before I submit.

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

Copy link

Member

terryjreedy commentedApr 25, 2022

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

Copy link

Member

terryjreedy commentedApr 25, 2022

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.)

Copy link

Member

merwok commentedApr 25, 2022

This format spec does funky alignment:

cpython/Lib/argparse.py

Line 521 in94d0853

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

AlexWaygood added the stdlibStandard Library Python modules in the Lib/ directory label

Apr 26, 2022

AbhigyanBose mentioned this pull request

Apr 27, 2022

Closed

Copy link

ContributorAuthor

AbhigyanBose commentedApr 27, 2022•
edited
Loading

@merwok and@terryjreedy

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

Merge branch 'main' ofhttps://github.com/python/cpythoninto fix-iss…

ca77e29

…ue-91832

merwok mentioned this pull request

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

Closed

Copy link

ContributorAuthor

AbhigyanBose commentedApr 28, 2022

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

merwok reviewed

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

merwok reviewed

Copy link

Member

merwok left a comment

Choose a reason for hiding this comment

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

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 therequired attribute is present in these versions too)

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

cd0e30e

…Li65.rstCo-authored-by: Éric <merwok@netwok.org>

Copy link

ContributorAuthor

AbhigyanBose commentedApr 28, 2022

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

merwok added the needs backport to 3.9 label

merwok added needs backport to 3.10

only security fixes

🤖 automerge labels

merwok approved these changes

bedevere-bot added awaiting merge and removed awaiting review labels

Copy link

Contributor

miss-islington commentedApr 28, 2022

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

miss-islington merged commit4ed3900 intopython:main

Copy link

Contributor

miss-islington commentedApr 28, 2022

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

bedevere-bot removed awaiting merge needs backport to 3.10only security fixes labels

Copy link

bedevere-bot commentedApr 28, 2022

GH-92021 is a backport of this pull request to the3.10 branch.

Copy link

bedevere-bot commentedApr 28, 2022

GH-92022 is a backport of this pull request to the3.9 branch.

bedevere-bot removed the needs backport to 3.9 label

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

pythongh-91832: Add 'required' attr to argparse.Action repr (pythonGH…

1bb49ac

…-91841)GH- Adding 'required' to names in Lib.argparse.Actionpythongh-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 commit4ed3900)Co-authored-by: Abhigyan Bose <abhigyandeepbose@gmail.com>

miss-islington added a commit that referenced this pull request

gh-91832: Add 'required' attr to argparse.Action repr (GH-91841)

fa87c36

GH- Adding 'required' to names in Lib.argparse.Actiongh-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 commit4ed3900)Co-authored-by: Abhigyan Bose <abhigyandeepbose@gmail.com>

miss-islington added a commit that referenced this pull request