Skip to content

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

Merged
isidentical merged 12 commits into
python:masterfrom
Jackenmen:pygettext_add_fstrings_support
Nov 9, 2020
Merged

bpo-36310: Allow pygettext.py to detect calls to gettext in f-strings.#19875
isidentical merged 12 commits into
python:masterfrom
Jackenmen:pygettext_add_fstrings_support

Conversation

@Jackenmen

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

Copy link
Copy Markdown
Contributor

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 October 18, 2020 07:29
@taleinat

taleinat commented Oct 18, 2020

Copy link
Copy Markdown
Contributor

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 reviewed changes

@isidentical isidentical left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Comment thread Tools/i18n/pygettext.py Outdated
Jackenmen and others added 2 commits October 18, 2020 19:17
@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 commented Oct 18, 2020

Copy link
Copy Markdown
Contributor Author

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

isidentical
isidentical reviewed Oct 18, 2020
View reviewed changes
Comment thread Tools/i18n/pygettext.py Outdated
@taleinat

taleinat commented Oct 19, 2020

Copy link
Copy Markdown
Contributor

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

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

@taleinat

taleinat commented Oct 19, 2020

Copy link
Copy Markdown
Contributor

@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

Jackenmen commented Oct 20, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 reviewed changes

@isidentical isidentical left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Comment thread Lib/test/test_tools/test_i18n.py Outdated
Comment thread Tools/i18n/pygettext.py Outdated
Comment thread Misc/NEWS.d/next/Tools-Demos/2020-05-03-01-30-46.bpo-36310.xDxxwY.rst Outdated
@isidentical
isidentical merged commit bfc6b63 into python:master Nov 9, 2020
@isidentical

isidentical commented Nov 9, 2020

Copy link
Copy Markdown
Member

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 November 9, 2020 22:52
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@isidentical isidentical isidentical approved these changes
@ericvsmith ericvsmith Awaiting requested review from ericvsmith

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Jackenmen @taleinat @isidentical @the-knights-who-say-ni @bedevere-bot