Add some missing translator comments and check for missing translator comments in future#7492
Conversation
141fc62 to
a60a8c7
Compare
… comments in future. 1. Add a few missing translator comments inspired by reports from translators. 2. Add some code (`checkPot`) to check for missing translator comments in the translation template (pot). For now, we have a set of expected failures, since fixing all existing 87 messages that don't have translator comments is going to take some work. However, we want to fail for any new messages that get introduced without comments. 3. Run `checkPot` as part of `scons tests`. This means tests (and thus builds) will fail so we learn about these problems early. 4. `checkPot` can also be run alone with `scons checkPot`. 5. Now that `scons tests` runs `checkPot` (which depends on `pot`), don't explicitly run `scons pot` on AppVeyor.
|
@michaelDCurran ended up fixing the string in #7258 in master 890528b. I've modified this PR accordingly. |
feerrenrut
left a comment
There was a problem hiding this comment.
Looks good overall, just few comments.
| # Existing messages that we know don't have translator comments yet. | ||
| # Ideally, all of these should get translator comments, | ||
| # but this is not realistic right now. | ||
| # A message should be removed from here once a translator comment is added for it. |
There was a problem hiding this comment.
Does the test fail if the exception is incorrectly specified? EG if someone adds a translator comment for one of these, and forgets to remove it from the exception list?
There was a problem hiding this comment.
Found out it does, could you update this comment to specify that it does?
| hasComment = True | ||
| continue | ||
| if line.startswith("#: "): | ||
| # Example: "#: NVDAObjects\window\winword.py:1322" |
There was a problem hiding this comment.
I'm confused by this, is this saying go and look at winword.py line 1322 for an example, or is the path part of the example? If so (the path case) it might be handy to add some further explanation text. EG "Sometimes we include paths in the translator comments, and so we must handle these because..."
| expectedErrors += 1 | ||
| continue | ||
| if hasComment and isExpectedError: | ||
| error = ("Message has translator comment, but one wasn't expected.\n" |
There was a problem hiding this comment.
Oh, turns out you do report this case. Great!
| # Strip the "#: " prefix (3 chars). | ||
| sourceLines.append(line[3:]) | ||
| continue | ||
| if line.startswith('msgctxt "'): |
There was a problem hiding this comment.
It's probably assumed knowledge for this, but what are: msgctxt, msgid, msgstr. Are these concepts from some other library module? Either a brief explanation of what they are or a pointer to where to find out about them could be handy for someone who comes to maintain this without that background knowledge.
|
@feerrenrut, thanks; I think I've addressed your review comments... with
code comments. :)
|
… comments in future. Incubates #7492.
…slatable strings - Ensured `checkPot` does not mistake a lengthy `msgid` with the POT file header - Added 5 additional known messages without translators comment with reference to their location in the source - Added 1 missing translators comment - Linted 25 existing translators comment so that they now properly land in the POT file
Link to issue number:
None specifically, but inspired by an omition in #7258.
Summary of the issue:
All translatable strings should have translator comments explaining them to translators. However, this is sometimes forgotten and this is easy to miss in review.
Description of how this pull request fixes the issue:
checkPot) to check for missing translator comments in the translation template (pot). For now, we have a set of expected failures, since fixing all existing 87 messages that don't have translator comments is going to take some work. However, we want to fail for any new messages that get introduced without comments.checkPotas part ofscons tests. This means tests (and thus builds) will fail so we learn about these problems early.checkPotcan also be run alone withscons checkPot.scons testsrunscheckPot(which depends onpot), don't explicitly runscons poton AppVeyor.Testing performed:
checkPotto fail.checkPotto fail, but do get counted in the summary.EXPECTED_MESSAGES_WITHOUT_COMMENTSset. Tested that this causes an unexpected success and thatcheckPotfails as a result.scons testsrunscheckPot.scons tests unitTests=test_cursorManagerdoes not runcheckPot(since explicit tests were specified).Known issues with pull request:
None.
Change log entry:
Changes for developers: