Skip to content
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

Jump to bottom

bpo-36310: Allow pygettext.py to detect calls to gettext in f-strings. #19875

Merged
merged 12 commits into from Nov 9, 2020
Merged

bpo-36310: Allow pygettext.py to detect calls to gettext in f-strings. #19875

merged 12 commits into from Nov 9, 2020

Conversation

Jackenmen
Copy link
Contributor

@Jackenmen Jackenmen commented May 3, 2020 •
edited by bedevere-bot

Adds support for gettext calls in f-strings. This was implemented using AST to parse the f-string since it seems like the simplest way to do this.

There are two places in which we should probably print a warning (I put a comment in each) but I'm not sure what the warning should be as unlike for regular string cases, there's no specific unexpected token I can print warning about.

https://bugs.python.org/issue36310

@Jackenmen Jackenmen changed the title bpo-36310: pygettext.py - Add support for use of gettext in f-strings bpo-36310: Allow pygettext.py to detect calls to gettext in f-strings. May 3, 2020
@Jackenmen Jackenmen closed this Aug 22, 2020
@Jackenmen Jackenmen reopened this Aug 22, 2020
@taleinat taleinat requested a review from ericvsmith Oct 18, 2020
@taleinat
Copy link
Contributor

taleinat commented Oct 18, 2020

Closing and re-opening to re-run the CI tests.

@taleinat taleinat closed this Oct 18, 2020
@taleinat taleinat reopened this Oct 18, 2020
isidentical
isidentical reviewed Oct 18, 2020
View changes
Copy link
Sponsor Member

@isidentical isidentical left a comment

It would be also nice if you add a test case for this inside of test.test_i18n.Test_pygettext

Tools/i18n/pygettext.py Outdated Show resolved Hide resolved
Jackenmen and others added 2 commits Oct 18, 2020
@Jackenmen @isidentical
Ignore Call.func if it's not Name or Attribute
758a55f 
Co-authored-by: Batuhan Taskaya <batuhanosmantaskaya@gmail.com>
@Jackenmen
Add some test cases
309245d
@Jackenmen Jackenmen force-pushed the pygettext_add_fstrings_support branch from 624d4d1 to 309245d Compare Oct 18, 2020
@Jackenmen
Copy link
Contributor Author

Jackenmen commented Oct 18, 2020

I added a few test cases and fixed the mentioned issue with node types.

isidentical
isidentical reviewed Oct 18, 2020
View changes
Tools/i18n/pygettext.py Outdated Show resolved Hide resolved
@taleinat
Copy link
Contributor

taleinat commented Oct 19, 2020

I've just pushed an update replacing break with continue, thanks :)

That seems like something that at least one test case should cover.

@taleinat
Copy link
Contributor

taleinat commented Oct 19, 2020

@jack1142, just wanted to thank you for your work on this! I definitely think it would be a boon to have this in Python.

@Jackenmen
Copy link
Contributor Author

Jackenmen commented Oct 20, 2020 •
edited

Okay, I added a few tests based on the paths where parsing is supposed to ignore the nodes including the one I mentioned above.
I also added myself to Misc/ACKS as I happened to read dev guide again and I think I can add myself there, let me know if I misunderstood something though.

While I'm commenting already, I just wanted to remind that I have 2 comments in the code in places where I think we should be printing some warning, so that the output contains information about incorrect usage of _() in f-strings similarly to how it does that outside of f-strings. I'm guessing these weren't full reviews yet and someone will probably respond to that, but I wouldn't want those to be missed so I'm mentioning it just in case.

@jack1142, just wanted to thank you for your work on this! I definitely think it would be a boon to have this in Python

I'm glad you like it 😄

isidentical
isidentical approved these changes Nov 5, 2020
View changes
Copy link
Sponsor Member

@isidentical isidentical left a comment

I had 3 very little comments, but overall LGTM. Thanks for the work @jack1142!

Lib/test/test_tools/test_i18n.py Outdated Show resolved Hide resolved
Tools/i18n/pygettext.py Outdated Show resolved Hide resolved
Misc/NEWS.d/next/Tools-Demos/2020-05-03-01-30-46.bpo-36310.xDxxwY.rst Outdated Show resolved Hide resolved
Jackenmen and others added 3 commits Nov 6, 2020
@isidentical isidentical merged commit bfc6b63 into python:master Nov 9, 2020
3 checks passed
@isidentical
Copy link
Sponsor Member

isidentical commented Nov 9, 2020

Thanks, @jack1142 for your patch, and fast responses to reviews! Hope to see more from you

@Jackenmen Jackenmen deleted the pygettext_add_fstrings_support branch Nov 9, 2020
@Jackenmen Jackenmen mentioned this pull request Feb 12, 2021
Closed
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@Jackenmen @isidentical @adorilson
bpo-36310: Allow pygettext.py to detect calls to gettext in f-strings. (
19c24a6 
pythonGH-19875)

Adds support to Tools/i18n/pygettext.py for gettext calls in f-strings. This process is done by parsing the f-strings, processing each value, and flagging the ones which contain a gettext call.

Co-authored-by: Batuhan Taskaya <batuhanosmantaskaya@gmail.com>
@Jackenmen Jackenmen mentioned this pull request Feb 20, 2022
Merged
@Infernio Infernio mentioned this pull request Jul 13, 2022
Open
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isidentical isidentical

@ericvsmith ericvsmith

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@Jackenmen @taleinat @isidentical @the-knights-who-say-ni @bedevere-bot