Skip to content
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 initial support for ICU MessageFormat. #6582

Merged
merged 7 commits into from Oct 27, 2021
Merged

Conversation

SirStendec
Copy link
Contributor

@SirStendec SirStendec commented Sep 22, 2021

Proposed changes

This pull request implements a new check for ICU MessageFormat messages. This check:

  • Parses ICU MessageFormat messages, performing syntax validation in the process
  • Checks that placeholders in source and translations match in name
  • Checks that placeholders in source and translations match in type
  • Checks that sub-message keys are valid for plural placeholders.
  • Checks that sub-message keys in translations are present in the source for select placeholders.
  • Supports basic XML tag placeholders, which are used by some libraries for rich content with ICU.
  • Can be configured via translation flags, letting users tweak the behavior to fit their strings better.

Specifically, this pull request:

  1. Adds the aforementioned check as weblate.checks.icu.ICUMessageFormatCheck
  2. Adds a dependency on the ICU MessageFormat parsing class pyicumessageformat which was written for this purpose.
  3. Adds unit tests for the new check (sitting at 85% coverage for icu.py)
  4. Documents the new check and its associated flags in user/checks.rst and admin/checks.rst

This pull request does not:

Checklist

  • Lint and unit tests pass locally with my changes.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added documentation to describe my feature.
  • I have squashed my commits into logic units.
  • I have described the changes in the commit messages.

Other information

Screenshot of a syntax error:

image

* Implements a fairly comprehensive checking class.
* Adds documentation about the check to admin/checks.rst and user/checks.rst
* Includes a new dependency on `pyicumessageformat`.
@codecov
Copy link

codecov bot commented Sep 23, 2021

Codecov Report

Merging #6582 (15851c5) into main (0b5121d) will decrease coverage by 0.00%.
The diff coverage is 90.55%.

❗ Current head 15851c5 differs from pull request most recent head 0a00274. Consider uploading reports for the commit 0a00274 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6582      +/-   ##
==========================================
- Coverage   91.41%   91.40%   -0.01%     
==========================================
  Files         624      626       +2     
  Lines       47023    47405     +382     
  Branches     5204     5292      +88     
==========================================
+ Hits        42984    43330     +346     
- Misses       2737     2765      +28     
- Partials     1302     1310       +8     
Impacted Files Coverage Δ
weblate/checks/models.py 85.91% <ø> (ø)
weblate/trans/models/unit.py 88.55% <0.00%> (-0.26%) ⬇️
weblate/checks/icu.py 86.12% <86.12%> (ø)
weblate/checks/flags.py 86.70% <100.00%> (+0.28%) ⬆️
weblate/checks/tests/test_checks.py 100.00% <100.00%> (ø)
weblate/checks/tests/test_icu_checks.py 100.00% <100.00%> (ø)
weblate/utils/requests.py 93.54% <0.00%> (-6.46%) ⬇️
weblate/checks/base.py 90.84% <0.00%> (+1.30%) ⬆️

@SirStendec
Copy link
Contributor Author

Sorry, I completely forgot about PrismJS highlighting in the first commit as I was thinking about refactoring the Live Preview feature.

Do you want me to restart this PR on a cleaner branch?

Copy link
Member

@nijel nijel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good! It would be great if you could improve test coverage for the code.

requirements.txt Outdated Show resolved Hide resolved
)

if result.get("tag_empty"):
yield _("XML Tag missing contents in translation: %s") % ", ".join(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please for more of the format_result branches?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be sure I'm not missing something here, did you mean "Can you please test for more [...]"?

Given the earlier comment about increasing test coverage, that's my assumption but I want to be sure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's it. Sorry for missing one word here....

@nijel nijel added this to the 4.9 milestone Oct 27, 2021
@nijel nijel linked an issue Oct 27, 2021 that may be closed by this pull request
@nijel nijel self-assigned this Oct 27, 2021
Copy link
Member

@nijel nijel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid nesting if statements.

weblate/checks/icu.py Outdated Show resolved Hide resolved
weblate/checks/icu.py Outdated Show resolved Hide resolved
@nijel nijel merged commit 36d291a into WeblateOrg:main Oct 27, 2021
@nijel
Copy link
Member

nijel commented Oct 27, 2021

Merged, thanks for your contribution!

nijel added a commit that referenced this pull request Oct 27, 2021
@nijel nijel mentioned this pull request Dec 15, 2021
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

weblate & icu format
2 participants