Skip to content

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

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

Closed
wants to merge 3 commits into from

Conversation

fatihkabakk
Copy link
Contributor

Closes #91832
Also there is another solution to just use self.__dict__ and ignore container, it gives the same list and seems to be better than creating and updating the list manually.
If self.__dict__ usage would be approved, I am volunteer to do the necessary change(s).

@ghost
Copy link

ghost commented Apr 22, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-bot
Copy link

Every change to Python requires a NEWS entry.

Please, add it using the blurb_it Web app or the blurb command-line tool.

@lamjay2704
Copy link

All set thanks

@fatihkabakk
Copy link
Contributor Author

Working on fixing the argparse test failure.

@fatihkabakk
Copy link
Contributor Author

Updated the tests, all passed on local Ubuntu!

@fatihkabakk
Copy link
Contributor Author

fatihkabakk commented Apr 22, 2022

Updated the tests, all passed on local Ubuntu!

However, when I fix test file it needs make patchcheck, when I do patchcheck whitespaces just get broken.
Don't know what to do at this point, it's like an endless-loop.

@AbhigyanBose
Copy link
Contributor

AbhigyanBose commented Apr 22, 2022

I checked the last commit on this file(test_argparse.py). It was eafec26 , it doesn't seem like the same issue occurred there. So, we might have to check if make patchcheck has somehow changed recently.

I can't see the output of that commit's build either. So can't look into it for any clue.

@merwok
Copy link
Member

merwok commented Apr 28, 2022

Also there is another solution to just use self.dict and ignore container, it gives the same list and seems to be better than creating and updating the list manually.

But internal names could be picked up by mistake, so an explicit list seems fine to me.
(Although unwanted names would break tests, so the mistake could be fixed. But I prefer to keep the original author’s code here.)

Thanks for the PR. Because the code here is the same as #91841, I am closing this one so that reviews and approvals stay in one place. The contribution is appreciated nonetheless!

@merwok merwok closed this Apr 28, 2022
@fatihkabakk fatihkabakk deleted the fix-issue-91832 branch May 5, 2022 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The argparse Action class omits "required" for the "names" list
5 participants