Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.3k
gh-126700: pygettext: Support more gettext functions#126912
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Tools/i18n/pygettext.py Outdated
| DEFAULTKEYWORDS=', '.join(default_keywords) | ||
| EMPTYSTRING='' | ||
| __version__='1.6' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I bumped the version since this adds some new capabilities, but let me know if it's not needed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It makes sense if the script is separately distributed. But when it is the part of the Python distribution, I think that we should use the Python version. We can discuss this in a separate issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good point, I reverted that change. My original reasoning was that since the version is written to the POT file we might want to bump it up but I agree that it should use the Python version itself, not a separate version.
| f"{_('foo', 'bar')}" | ||
| ''')) | ||
| self.assertNotIn('foo',msgids) | ||
| self.assertIn('foo',msgids) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Bothxgettext andpybabel extract this and it would take a lot of effort to disallow this in general with the current extractor, so I'd leave this for now at least.
| # calculate all keywords | ||
| options.keywords.extend(default_keywords) | ||
| options.keywords= {kw: {0:'msgid'}forkwinoptions.keywords} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
--keyword still works but the keywords are assumed to be single-argument. A followup PR could add support for more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Great! I just took a look and will do a more detailed review tomorrow. So far I see two problems:
- It does not work correctly when the first argument ща
dgettextis a complex expression that includes parentheses or commas. Nested parentheses should be counted as in__suiteseen. - No warning is issued when the expected argument is not a string literal. Such bug were recently fixed in
argparse. The new i18nargparsetests would catch such bugs, becausepygettextemits warnings, but with this PR it silently ignores them.
That should be fixed now! I used the same mechanism that
I restored the warnings. I initially removed them in order to allow extraction of keyword arguments (e.g. Note that there are still a lot of edge cases when it comes to extraction which I didn't want to address in this PR. For instance, extraction of nested constructs such as |
Uh oh!
There was an error while loading.Please reload this page.
| argument '%(argument_name)s' is deprecated | ||
| can't open '%(filename)s': %(error)s | ||
| command '%(parser_name)s' is deprecated | ||
| conflicting option string: %s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Shouldmsgid_plural also be output? Or do this in the following PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We might also want to includemsgctxt even though I don't think any messages usepgettext currently. I was thinking instead of using a list of msgids, why not use the generated POT file?
We initially rejected that idea when adding the snapshots because of potentially changing line locations, but pygettext has an option to turn those off.
We could even add an option to not emit the header (pybabel has this for instance). Then the snapshots would only change if the strings themselves change.
Misc/NEWS.d/next/Tools-Demos/2024-11-16-20-47-20.gh-issue-126700.ayrHv4.rst OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Tools/i18n/pygettext.py Outdated
| DEFAULTKEYWORDS=', '.join(default_keywords) | ||
| EMPTYSTRING='' | ||
| __version__='1.6' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It makes sense if the script is separately distributed. But when it is the part of the Python distribution, I think that we should use the Python version. We can discuss this in a separate issue.
| self.__messages= {} | ||
| self.__state=self.__waiting | ||
| self.__data= [] | ||
| self.__data=defaultdict(str) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why use defaultdict?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
So that I can do this one-liner 😄 :
Uh oh!
There was an error while loading.Please reload this page.
Tools/i18n/pygettext.py Outdated
| eliftstringin')]}': | ||
| self.__enclosurecount-=1 | ||
| elifexpect_string_literal: | ||
| # We are inside an argument which is a translatable string and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this can be merged with the below. But I can be wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hmm I don't think it can, unless I am misunderstading what you mean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It is not so important now, after you moved the ugly part of the code intowarn_unexpected_token(), but you can avoid the code duplication by using earlierreturns.
ifttype==tokenize.OPandself.__enclosurecount==0:iftstring==')': ...returniftstring==',': ...returnifexpect_string_literal: ...# handle string literals, comments, etcelse: ...# handle parentheses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice! I wasn't considering early returns but I think it's more digestible this way. I updated the code and added more test cases :)
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Tools/i18n/pygettext.py Outdated
| eliftstringin'([{': | ||
| self.__enclosurecount+=1 | ||
| eliftstringin')]}': | ||
| self.__enclosurecount-=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
These are invalid ifexpect_string_literal is true.
I suspect that this does not work correctly for_('string'[i]).
See the comment below.
Tools/i18n/pygettext.py Outdated
| eliftstringin')]}': | ||
| self.__enclosurecount-=1 | ||
| elifexpect_string_literal: | ||
| # We are inside an argument which is a translatable string and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It is not so important now, after you moved the ugly part of the code intowarn_unexpected_token(), but you can avoid the code duplication by using earlierreturns.
ifttype==tokenize.OPandself.__enclosurecount==0:iftstring==')': ...returniftstring==',': ...returnifexpect_string_literal: ...# handle string literals, comments, etcelse: ...# handle parentheses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM. 👍
0a1944c intopython:mainUh oh!
There was an error while loading.Please reload this page.
…126912)Support multi-argument gettext functions: ngettext(), pgettext(), dgettext(), etc.
Uh oh!
There was an error while loading.Please reload this page.
pygettextcurrently only supports the single-argument form_('foo'). This can be extended via the--keywordCLI argument but all functions are assumed to be single-argument as well. That means nongettext,pgettext, etc..pygettextis currently not able to emitmsgctxtandmsgid_pluralat all.This PR fixes that by adding support for all standard gettext functions:
These can be extended using
--keyword=KEYWORDbut all such keywords are still assumed to be single-argument. Adding CLI support requires parsing thexgettextkeyword specifier format (e.g.pgettext:1c,2) which can be done in a followup PR.Adding support for other gettext functions means that pygettext now extracts calls that may seem a bit questionable such as
_(x="kwargs")but it is consistent with other extractors likexgettextandpybabel. This is also the price for using a token-based extractor. If we want to keep the code reasonably simple, we'll need to accept some false positives. I would eventually like to see pygettext switch to an AST-based extractor which would eliminate these issues.I tried to keep the diff minimal, but some larger changes were needed. Feedback welcome!