Skip to content

bpo-44864: Do not translate user-provided strings in ArgumentParser.add_subparsers() #27667

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Sep 24, 2024
Merged

bpo-44864: Do not translate user-provided strings in ArgumentParser.add_subparsers() #27667

merged 4 commits into from
Sep 24, 2024

Conversation

jdetrey
Copy link
Contributor

@jdetrey jdetrey commented Aug 8, 2021 •
edited
Loading

Call _() on argparse-provided strings only.

https://bugs.python.org/issue44864

@jdetrey
bpo-44864: Do not translate user-provided strings in `ArgumentParser.…
addd1c5 
…add_subparsers()`

Call `_()` on `argparse`-provided strings only.
@python python deleted a comment from the-knights-who-say-ni Aug 13, 2021
MaxwellDupre
MaxwellDupre approved these changes Apr 21, 2022
View reviewed changes
Copy link
Contributor

@MaxwellDupre MaxwellDupre left a comment

Choose a reason for hiding this comment

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

Ran 1670 tests in 8.544s
OK
Looks ok.

@jdetrey jdetrey mannequin mentioned this pull request Apr 10, 2022
Closed
iritkatriel
iritkatriel requested changes Nov 27, 2022
View reviewed changes
Copy link
Member

@iritkatriel iritkatriel left a comment

Choose a reason for hiding this comment

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

This is a behaviour change and, as such, requires a unit test to cover it.

If there are no tests for translations in argparse, those should be added and possibly we need to go over what does and what doesn't need to be translated.

CC @terryjreedy

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

@encukou
Copy link
Member

encukou commented Mar 19, 2024

I agree that this needs tests -- at least very specific ones for the changed cases.

The change itself is correct: argparse-provided strings should be translated, user-provided ones should not. The translation function (_) should only be used on string literals.

serhiy-storchaka
serhiy-storchaka approved these changes Sep 22, 2024
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

There are no currently any tests for gettext in argparse, and adding them requires a lot of effort. I do not think that this should be a blocker for this bugfix. You can open a new issue for adding tests for gettext in argparse.

@serhiy-storchaka
Copy link
Member

@iritkatriel, @encukou, are you fine with merging this PR without tests and creating a separate issue for adding tests for argparse+gettext?

@iritkatriel
Copy link
Member

Sure.

@serhiy-storchaka
Copy link
Member

Opened #124432.

@encukou
Copy link
Member

encukou commented Sep 24, 2024

Yes, I'm fine with that.

@serhiy-storchaka serhiy-storchaka enabled auto-merge (squash) September 24, 2024 18:54
@serhiy-storchaka
Copy link
Member

BTW, Tools/i18n/pygettext.py fails with the current code and passes with the fixed one.

@serhiy-storchaka serhiy-storchaka merged commit d3c76df into python:main Sep 24, 2024
34 checks passed
@serhiy-storchaka serhiy-storchaka mentioned this pull request Sep 25, 2024
Closed
@serhiy-storchaka serhiy-storchaka added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Sep 25, 2024
@miss-islington-app
Copy link

Thanks @jdetrey for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Thanks @jdetrey for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 25, 2024
@jdetrey @miss-islington
bpo-44864: Do not translate user-provided strings in ArgumentParser.a…
8a3a7a4 
…dd_subparsers() (pythonGH-27667)

Call _() on literal strings only.
(cherry picked from commit d3c76df)

Co-authored-by: Jérémie Detrey <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Sep 25, 2024

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 25, 2024
@jdetrey @miss-islington
bpo-44864: Do not translate user-provided strings in ArgumentParser.a…
d201ce0 
…dd_subparsers() (pythonGH-27667)

Call _() on literal strings only.
(cherry picked from commit d3c76df)

Co-authored-by: Jérémie Detrey <[email protected]>
@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Sep 25, 2024
@bedevere-app
Copy link

bedevere-app bot commented Sep 25, 2024

GH-124506 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 Sep 25, 2024
serhiy-storchaka pushed a commit that referenced this pull request Sep 25, 2024
@miss-islington @jdetrey
[3.12] bpo-44864: Do not translate user-provided strings in ArgumentP…
306368c 
…arser.add_subparsers() (GH-27667) (GH-124505)

Call _() on literal strings only.
(cherry picked from commit d3c76df)

Co-authored-by: Jérémie Detrey <[email protected]>
Yhg1s pushed a commit that referenced this pull request Sep 30, 2024
@miss-islington @jdetrey
[3.13] bpo-44864: Do not translate user-provided strings in ArgumentP…
6425443 
…arser.add_subparsers() (GH-27667) (#124506)

bpo-44864: Do not translate user-provided strings in ArgumentParser.add_subparsers() (GH-27667)

Call _() on literal strings only.
(cherry picked from commit d3c76df)

Co-authored-by: Jérémie Detrey <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iritkatriel iritkatriel iritkatriel requested changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@MaxwellDupre MaxwellDupre MaxwellDupre approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@jdetrey @bedevere-bot @encukou @serhiy-storchaka @iritkatriel @MaxwellDupre @the-knights-who-say-ni @ezio-melotti