gh-130453: pygettext: Extend support for specifying custom keywords #130463
Conversation
| raise ValueError(f'Invalid keyword spec {spec!r}: ' | ||
| 'msgctxt cannot appear without msgid') | ||
|
|
||
| return name, {v: k for k, v in result.items()} |
There was a problem hiding this comment.
Would it be simpler to build result in that form from the beginning?
There was a problem hiding this comment.
I did that in d861c84, let me know if you like it better like that
There was a problem hiding this comment.
It was just a question. I am fine with both variants.
There was a problem hiding this comment.
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.
Actually, I tried implementing some followup work on top of this PR (support for the t 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.
So I did the right thing by letting the PR lie down for two days. 😄
There was a problem hiding this comment.
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) for spec in options.keywords) | ||
| except ValueError as e: | ||
| raise SystemExit(e) |
There was a problem hiding this comment.
Other errors cause print() + sys.exit(1).
There was a problem hiding this comment.
Indeed, though I believe raise SystemExit(s) is functionally equivalent to print(..., 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 to print+sys.exit?
There was a problem hiding this comment.
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.
Updated in 18d29cb to use print+sys.exit
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) for spec in options.keywords) | ||
| except ValueError as e: | ||
| raise SystemExit(e) |
There was a problem hiding this comment.
Yes, it just for consistency (also, sys.exit() allows to set different return codes, but this is not used here).
| raise ValueError(f'Invalid keyword spec {spec!r}: ' | ||
| 'msgctxt cannot appear without msgid') | ||
|
|
||
| return name, {v: k for k, v in result.items()} |
There was a problem hiding this comment.
It was just a question. I am fine with both variants.
This addresses the first point in #130453
It is now possible to use the full keyword 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!