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-130453: pygettext: Extend support for specifying custom keywords#130463
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
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 do not think that it was necessary to go so far with detecting errors and generating error reports. Garbage in -- garbage out. The parsing code could be 2 or 3 times smaller without this. But if you already implemented this, it is fine.
LGTM.
Uh oh!
There was an error while loading.Please reload this page.
| raiseValueError(f'Invalid keyword spec{spec!r}: ' | ||
| 'msgctxt cannot appear without msgid') | ||
| returnname, {v:kfork,vinresult.items()} |
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.
Would it be simpler to buildresult in that form from the beginning?
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 did that ind861c84, let me know if you like it better like that
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 was just a question. I am fine with both variants.
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 just wanted to let you see the difference :) I don't have a strong preference either, let's stick with the current version, then?
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.
Actually, I tried implementing some followup work on top of this PR (support for thet specifier, multiple keywords with the same funcname) and it's better to use the original representation because the diff in the followup PRs will be smaller.
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 I did the right thing by letting the PR lie down for two days. 😄
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.
Yes, good call 🙂 And thanks for your super thorough reviews! It's really appreciated
Tools/i18n/pygettext.py Outdated
| try: | ||
| options.keywords=dict(parse_spec(spec)forspecinoptions.keywords) | ||
| exceptValueErrorase: | ||
| raiseSystemExit(e) |
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.
Other errors causeprint() +sys.exit(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.
Indeed, though I believeraise SystemExit(s) is functionally equivalent toprint(..., file=sys.stderr) +sys.exit(1) and since it's shorter and the intent is clearer, I thought I'd start using that instead.
Though if you prefer to be consistent, I can change it toprint+sys.exit?
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.
Yes, it just for consistency (also,sys.exit() allows to set different return codes, but this is not used here).
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.
Updated in18d29cb to useprint+sys.exit
Uh oh!
There was an error while loading.Please reload this page.
Honestly, if you prefer it without the detailed error messages, I am fine with removing them. Just let me know! My thinking for adding them was that most people using this will not be that familiar with the syntax and for them, it's better to show a descriptive error rather than fail silently, but as I said, if you prefer to have simpler code, that's also ok! |
Tools/i18n/pygettext.py Outdated
| try: | ||
| options.keywords=dict(parse_spec(spec)forspecinoptions.keywords) | ||
| exceptValueErrorase: | ||
| raiseSystemExit(e) |
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.
Yes, it just for consistency (also,sys.exit() allows to set different return codes, but this is not used here).
| raiseValueError(f'Invalid keyword spec{spec!r}: ' | ||
| 'msgctxt cannot appear without msgid') | ||
| returnname, {v:kfork,vinresult.items()} |
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 was just a question. I am fine with both variants.
44213bc intopython:mainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
This addresses the first point in#130453
It is now possible to use the fullkeyword spec syntax (except for
t, that will be added later) to specify keywords:I tried to match the behaviour of xgettext and babel but neither seem to do much validation for the keyword specs.
xgettext, for instance, does not allow
foo:1c,2c(context specified twice) norfoo:1,1c(msgidandmsgctxthave the same index) but it does (weirdly) allowfoo:1,1(same index formsgidandmsgid_plural), whereas it outright crashes with a double free forfoo:1,1,2c.This PR properly validates the keyword specs in order to be consistent and provide helpful error messages to the user.
Feedback welcome!