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

Merged
merged 2 commits into from Aug 29, 2017

Conversation

Projects
None yet
3 participants
@jcsteh
Contributor

jcsteh commented Aug 14, 2017

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:

  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.

Testing performed:

  1. Tested that unexpected errors cause checkPot to fail.
  2. Tested that expected errors don't cause checkPot to fail, but do get counted in the summary.
  3. Added a message that already has a translator comment to the EXPECTED_MESSAGES_WITHOUT_COMMENTS set. Tested that this causes an unexpected success and that checkPot fails as a result.
  4. Tested that scons tests runs checkPot.
  5. Tested that scons tests unitTests=test_cursorManager does not run checkPot (since explicit tests were specified).
  6. Tested that an unexpected error causes an AppVeyor build to fail.

Known issues with pull request:

None.

Change log entry:

Changes for developers:

- "scons tests" now checks that translatable strings have translator comments. You can also run this alone with "scons checkPot". (#7492)

@jcsteh jcsteh requested a review from feerrenrut Aug 14, 2017

Add some missing translator comments and check for missing translator…
… 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.
@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Aug 14, 2017

Contributor

@michaelDCurran ended up fixing the string in #7258 in master 890528b. I've modified this PR accordingly.

Contributor

jcsteh commented Aug 14, 2017

@michaelDCurran ended up fixing the string in #7258 in master 890528b. I've modified this PR accordingly.

@feerrenrut

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.

This comment has been minimized.

@feerrenrut

feerrenrut Aug 14, 2017

Contributor

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?

@feerrenrut

feerrenrut Aug 14, 2017

Contributor

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?

This comment has been minimized.

@feerrenrut

feerrenrut Aug 14, 2017

Contributor

Found out it does, could you update this comment to specify that it does?

@feerrenrut

feerrenrut Aug 14, 2017

Contributor

Found out it does, could you update this comment to specify that it does?

Show outdated Hide outdated tests/checkPot.py Outdated
expectedErrors += 1
continue
if hasComment and isExpectedError:
error = ("Message has translator comment, but one wasn't expected.\n"

This comment has been minimized.

@feerrenrut

feerrenrut Aug 14, 2017

Contributor

Oh, turns out you do report this case. Great!

@feerrenrut

feerrenrut Aug 14, 2017

Contributor

Oh, turns out you do report this case. Great!

# Strip the "#: " prefix (3 chars).
sourceLines.append(line[3:])
continue
if line.startswith('msgctxt "'):

This comment has been minimized.

@feerrenrut

feerrenrut Aug 14, 2017

Contributor

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

feerrenrut Aug 14, 2017

Contributor

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.

@jcsteh

This comment has been minimized.

Show comment
Hide comment
@jcsteh

jcsteh Aug 14, 2017

Contributor
Contributor

jcsteh commented Aug 14, 2017

jcsteh added a commit that referenced this pull request Aug 15, 2017

@jcsteh jcsteh merged commit 5b636a3 into master Aug 29, 2017

@nvaccessAuto nvaccessAuto added this to the 2017.4 milestone Aug 29, 2017

@jcsteh jcsteh deleted the transComments branch Aug 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment