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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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. |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found out it does, could you update this comment to specify that it does?
tests/checkPot.py
Outdated
hasComment = True | ||
continue | ||
if line.startswith("#: "): | ||
# Example: "#: NVDAObjects\window\winword.py:1322" |
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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.checkPot
as part ofscons tests
. This means tests (and thus builds) will fail so we learn about these problems early.checkPot
can also be run alone withscons checkPot
.scons tests
runscheckPot
(which depends onpot
), don't explicitly runscons pot
on AppVeyor.Testing performed:
checkPot
to fail.checkPot
to fail, but do get counted in the summary.EXPECTED_MESSAGES_WITHOUT_COMMENTS
set. Tested that this causes an unexpected success and thatcheckPot
fails as a result.scons tests
runscheckPot
.scons tests unitTests=test_cursorManager
does not runcheckPot
(since explicit tests were specified).Known issues with pull request:
None.
Change log entry:
Changes for developers: