glaterza commented on PR #44395:
URL: https://github.com/apache/superset/pull/44395#issuecomment-5731997082
Rebased onto current and added the regression test you asked for,
@sadpandajoe.
## The test
— five tests covering both halves of your comment:
| Test | Guards |
|---|---|
| flags contract | `--sort-output` present, `--sort-by-msgid` absent,
`--no-wrap` + `--no-location` kept |
| fail-fast | the msgcat call ends with `\|\| exit 1` |
| ordering | normalization runs *before* `pybabel update` |
| fixture round-trip | an unsorted, width-wrapped, location-carrying `.pot`
comes out sorted, unwrapped and location-free |
| **end-to-end refusal** | an injected failing `msgcat` makes the real
script exit non-zero **without reaching `pybabel update`** |
Two choices worth flagging, since both affect whether the test is worth
having:
**The msgcat command is parsed out of the script, not restated in the
test.** A test carrying its own copy of the command would have gone on passing
while the script broke — which is exactly the shape of the bug being fixed.
Deleting the `msgcat` line from the script fails all five tests.
**The end-to-end test needs neither babel nor gettext.** It stubs `pybabel`
and `msgcat` onto `PATH` and runs a *copy* of the real script placed so its
`BASH_SOURCE`-derived `ROOT_DIR` points at a tmpdir, so nothing in the repo is
touched. Only the fixture test needs real gettext, and it skips with a reason
when `msgcat` is absent — so the two guarantees you care about run on every
runner regardless.
I verified the tests actually bite by reintroducing each defect one at a
time:
```
--sort-output -> --sort-by-msgid 2 failed, 3 passed
drop `|| exit 1` 2 failed, 3 passed
drop --no-wrap 2 failed, 3 passed
delete the msgcat step entirely 5 failed
```
then restored the script byte-identical — 5 passed.
## The rebase
The `.pot` conflicted, as a generated file will. Rather than hand-resolve
it, I rebuilt the branch on current `master` and **regenerated** the template
by applying the now-working normalization to the one on `master`, so the commit
is reproducible without a full `pybabel extract`:
```
head -17 messages.pot > license # the ASF header the script re-prepends
tail -n +18 messages.pot > body
msgcat --sort-output --no-wrap --no-location body -o body.norm
cat license body.norm > messages.pot
```
Measured before → after:
| | before | after |
|---|---|---|
| msgids | 5124 | 5124 |
| location comments (`#:`) | 1 | 0 |
| width-wrapped lines | 1294 | 0 |
| msgid sort inversions | 7 | 0 |
| `do-not-translate` markers | 27 | 27 |
| `no-python-format` flags | 3 | 3 |
The 99 continuation lines that remain are newline-driven (msgids containing
`\n`), which `--no-wrap` does not govern. Sort inversions were compared
**bytewise on decoded msgids**, the way msgcat actually sorts — comparing the
escaped source form gives false positives on `\n` and `\"` prefixes. `msgfmt
--check-format` accepts the result.
**One trap for anyone regenerating this by hand:** don't apply the script's
trailing-line `sed '$ d'` chop to msgcat's output. That chop exists for a babel
quirk ([python-babel#799](https://github.com/python-babel/babel/issues/799))
where `pybabel update` leaves a trailing blank line; msgcat doesn't, so the
chop eats a real `msgstr ""` and `msgfmt` then fails with `missing 'msgstr'
section`. I hit it, which is why it's in the commit message.
Pre-commit is clean on all three commits (mypy and ruff included).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
