Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-130453: pygettext: Allow specifying multiple keywords with the same function name#131380

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

Merged
serhiy-storchaka merged 13 commits intopython:mainfromtomasr8:pygettext-mult-keywords
Apr 10, 2025

Conversation

@tomasr8
Copy link
Member

@tomasr8tomasr8 commentedMar 17, 2025
edited by bedevere-appbot
Loading

There has been some recent work making the custom keyword option (--keyword) more useful and flexible (#130463 &#130709)

One thing that is still missing, is the ability to specify multiple keywords with the same function name. For examplepython pygettext.py --keyword=foo --keyword=foo:1,2. This should extract calls likefoo('bar') as regular gettext and calls likefoo('bar', 'bars', 1) as ngettext.

Specifying multiple keywords is already supported by both xgettext and babel and it is in general quite useful to be able to do this for cases when you have an overloaded gettext function which behaves differently based on the number of arguments passed to it.

This PR allows you to pass multiple keywords with the same function name, for example--keyword=foo --keyword=foo:1,2 is allowed and does what one would expect.

This does not let you distinguish between e.g.ngettext andpgettext since there is no way to specify the total number of arguments which is done with thet specifier (e.g.foo:1c,2,2t for pgettext andfoo:1,2,3t for ngettext). This can be added later.

Feedback welcome :)

Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Some warning/error messages can be printed incorrectly or repeatedly.

Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

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

It is not easy to produce a correct and helpful error message.

f'arguments are not allowed in gettext calls',file=sys.stderr)
returnFalse
return (f'***{self.filename}:{node.lineno}: Variable positional '
f'arguments are not allowed in gettext calls')

Choose a reason for hiding this comment

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

Suggested change
f'arguments are not allowed in gettext calls')
f'arguments are not allowed in gettext calls')

Choose a reason for hiding this comment

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

This error will still be printed multiple times.

Comment on lines 502 to 504
return (f'***{self.filename}:{node.lineno}: Expected at least '
f'{max_index+1} positional argument(s) in gettext call, '
f'got{len(node.args)}')

Choose a reason for hiding this comment

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

With--keyword=_:1 --keyword=_:2, multiple errors will be printed for_(): "Expected at least 1 positional argument(s)...", "Expected at least 2 positional argument(s)...".

Comment on lines 567 to 568
b'*** test.py:1: Expected a string constant for argument 1, got x\n'
b'*** test.py:1: Expected a string constant for argument 2, got 42\n'

Choose a reason for hiding this comment

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

This is misleading, since only one of arguments needs to be a string.

@tomasr8
Copy link
MemberAuthor

It is not easy to produce a correct and helpful error message.

Indeed :/ I'm still trying to figure out the best way to show the messages. I'll make the PR a draft for now while I experiment with it

@tomasr8tomasr8 marked this pull request as draftMarch 19, 2025 21:42
@tomasr8
Copy link
MemberAuthor

Ok so I think the main issue with the error messages is that it is not clear that pygettext is trying to match multiple specs and each of the error message relates to a different spec but for the same function call. I think we can make the message more structured in case there are multiple specs for the same function name. Something like this:

*** test.py:1: No keywords matched gettext call "_":        keyword="_": Expected a string constant for argument 1, got x        keyword="_:2": Expected a string constant for argument 2, got 42

The 'main' message makes it clear there are multiple specs and none of them matched. Then each sub-message is prefixed with a particular spec and shows the error for that spec. This is in my opinion clearer and makes it easier for the user to figure out where the problem is.

@serhiy-storchaka
Copy link
Member

Whatxgettext does?

@tomasr8
Copy link
MemberAuthor

xgettext does not print anything when it can't extract a message, which is also an option I suppose, though pygettext has had these error messages since the beginning so I don't know if it's a good idea to get rid of them?

tomasr8and others added4 commitsMarch 20, 2025 22:37
@tomasr8
Copy link
MemberAuthor

I implemented the example I showed above. It collects errors from all keyword specs for a given func name and then prints them all in a structured way that is hopefully both clear and helpful. I'd like to know what you think!

Alternatively, we could just get rid of the error messages which would simplify the code. Though as I wrote before, pygettext has always had these error messages and I think they are useful as they help you find i18n issues in your code.

serhiy-storchaka reacted with thumbs up emoji

@tomasr8tomasr8 marked this pull request as ready for reviewMarch 23, 2025 20:21
Copy link
Member

@serhiy-storchakaserhiy-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! 👍

tomasr8 reacted with heart emoji
tomasr8and others added2 commitsApril 10, 2025 09:34
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@serhiy-storchakaserhiy-storchakaenabled auto-merge (squash)April 10, 2025 08:16
@serhiy-storchakaserhiy-storchaka merged commitb6760b7 intopython:mainApr 10, 2025
39 checks passed
@tomasr8tomasr8 deleted the pygettext-mult-keywords branchApril 10, 2025 11:08
seehwan pushed a commit to seehwan/cpython that referenced this pull requestApr 16, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@serhiy-storchakaserhiy-storchakaserhiy-storchaka approved these changes

@AA-TurnerAA-TurnerAwaiting requested review from AA-Turner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tomasr8@serhiy-storchaka

[8]ページ先頭

©2009-2025 Movatter.jp