bpo-38821: Fix crash in argparse when using gettext

[federicobond]

https://bugs.python.org/issue38821

[the-knights-who-say-ni]

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@federicobond

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

[brandtbucher]

Thanks for your time, and welcome to CPython @federicobond!

I assume that you've seen the bot's message about the CLA?

[federicobond]

Hi there @brandtbucher, yes. Waiting for the CLA to get accepted! Should I include a changelog in the PR?

[brandtbucher]

Yes please! You can do so easily here.

[federicobond]

Done! I was not feeling particularly inspired writing the news entry, so feel free to suggest changes.

[rhettinger]

Please add a test.

[federicobond]

I could not find any existing test exercising locale support in argparse. What would this test assert?

[taleinat]

I could not find any existing test exercising locale support in argparse. What would this test assert?

I suggest testing various possible values for nargs, including all of the special ones, when using gettext.

[federicobond]

I think adding those tests would be a great idea but none of them would fail prior to this change, so I am not sure we should bundle them together.

A proper regression test for this change needs to load a custom catalog, bind it to the global domain, etc.

[brandtbucher]

Just a bit of clarification on the NEWS entry:

[taleinat]

A proper regression test for this change needs to load a custom catalog, bind it to the global domain, etc.

We could also monkey-patch argparse._ and argparse.ngettext using unittest.mock.patch, which should be simpler to set up.

That being said, this PR is an obvious fix and we apparently have no existing tests for the use of gettext in argparse, so I realize that demanding that you add such tests is rather unfair. Let me know whether you'd like to try, and I'll take care of things even if you don't.

[federicobond]

@taleinat sure, I can do that. Do you think those tests should accompany this PR or should I open a new one? I would rather submit another PR so we can discuss them separately and not stall this changeset, but whatever works best for the maintainers is good for me too.

[taleinat]

@federicobond, yes, let's go with a separate issue and PR for adding such tests.

[miss-islington]

Thanks @federicobond for the PR, and @taleinat for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖

[miss-islington]

I'm having trouble backporting to 3.8. Reason: 'Error 110 while writing to socket. Connection timed out.'. Please retry by removing and re-adding the needs backport to 3.8 label.

[bedevere-bot]

GH-17288 is a backport of this pull request to the 3.7 branch.

[miss-islington]

Thanks @federicobond for the PR, and @taleinat for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

[bedevere-bot]

GH-17289 is a backport of this pull request to the 3.8 branch.

[brandtbucher]

Congrats on your first CPython contribution @federicobond! 🍾

Looking forward to seeing more from you in the future.

[federicobond]

Thank you @brandtbucher & co! I can see that you have all put a lot of effort into making this a pleasurable experience, definitely makes me want to contribute again 🎉