Add support for XLIFF placeholders #2364

PowerKiKi · 2018-10-31T11:27:52Z

This is a work in progress and unit tests will be added and squashed when ready. But I wanted to double-check if I am going in the right direction before progressing further.

Given this XLIFF unit:

<trans-unit id="id-1">
    <source xml:space="preserve">source "<x id="INTERPOLATION" equiv-text="{{ angularExpression }}"/>", source.</source>
    <target xml:space="preserve">target "<x id="INTERPOLATION" equiv-text="{{ angularExpression }}"/>", target.</target>
</trans-unit>

I store in DB the placeholder preserved as XML. So something like the following (after translation of target), note the absence of flags:

In Weblate, it will look like that:

Interestingly I did not implement any checks, but the placeholder are still highlighted. So I guess we don't need to do anything for that part ?

Does it look good to you ? should I keep going by fixing/adding unit tests ?

Fixes #490
Fixes #1535

Before submitting pull request, please ensure that:

Every commit has message which describes it
Every commit is needed on it's own, if you have just minor fixes to previous commits, you can squash them
Any code changes come with testcase
Any new functionality is covered by user documentation

codeclimate · 2018-10-31T13:05:15Z

weblate/formats/ttkit.py

@@ -37,7 +37,7 @@

 from weblate.formats.base import FileUnit, FileFormat

-from weblate.trans.util import get_string, join_plural
+from weblate.trans.util import get_string, join_plural, string_to_rich, rich_to_string


Line too long (86 > 79 characters)

codeclimate · 2018-10-31T13:05:15Z

weblate/trans/util.py

+
+
+def string_to_rich(string):
+    """Transform a string containing XLIFF placeholders as XML into a rich content (StringElement)"""


Line too long (101 > 79 characters)

codeclimate · 2018-10-31T13:05:15Z

weblate/trans/util.py

+
+
+def rich_to_string(string_elements):
+    """Transform rich content (StringElement) into a string with placeholder kept as XML"""


Line too long (91 > 79 characters)

nijel · 2018-10-31T21:56:50Z

Thanks! Looks good generally, some remarks:

The new functions in utils probably should include xliff in name, as those are xliff specific.
You get the highlighting for generic XML markup, maybe that's good enough and there is no need to deal with the flags.
This seems to break more things than I expected, maybe rich_* properties are empty if there is no markup?

nijel · 2018-10-31T21:58:48Z

Most of the test failures are probably caused by this:

  File "/home/travis/build/WeblateOrg/weblate/weblate/formats/ttkit.py", line 75, in get_source
    return rich_to_string(self.unit.rich_source)
  File "/home/travis/build/WeblateOrg/weblate/weblate/trans/util.py", line 308, in rich_to_string
    result += string_without_wrapping_element
TypeError: must be str, not bytes

codecov · 2018-11-06T06:42:45Z

Codecov Report

Merging #2364 into master will increase coverage by 1.05%.
The diff coverage is 95.23%.

@@            Coverage Diff            @@
##           master   #2364      +/-   ##
=========================================
+ Coverage   90.95%     92%   +1.05%     
=========================================
  Files         396     394       -2     
  Lines       27590   27287     -303     
  Branches     3044    3027      -17     
=========================================
+ Hits        25094   25105      +11     
+ Misses       1662    1376     -286     
+ Partials      834     806      -28

Impacted Files	Coverage Δ
weblate/trans/util.py	`91.15% <100%> (+2.92%)`	⬆️
weblate/trans/tests/test_xliff_placeholders.py	`100% <100%> (ø)`
weblate/formats/ttkit.py	`94.34% <87.5%> (+3.65%)`	⬆️
weblate/trans/views/acl.py	`82% <0%> (-4%)`	⬇️
weblate/machinery/base.py	`87.26% <0%> (-3.48%)`	⬇️
weblate/trans/models/translation.py	`91.51% <0%> (-1.72%)`	⬇️
weblate/utils/checks.py	`65.93% <0%> (-1.72%)`	⬇️
weblate/trans/views/git.py	`75% <0%> (-0.48%)`	⬇️
weblate/trans/management/commands/__init__.py	`85.71% <0%> (-0.43%)`	⬇️
weblate/trans/models/change.py	`91.37% <0%> (-0.15%)`	⬇️
... and 90 more

PowerKiKi · 2018-11-07T04:12:25Z

I fixed style and unit tests (hopefully) and added new tests to cover new code.

Could you please have a second look ?

nijel · 2018-11-09T15:42:53Z

Thanks, it looks better now. Still there is one failure on the xliff import:

======================================================================
FAIL: test_import_xliff (weblate.trans.tests.test_files.ImportPHPMonoTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/WeblateOrg/weblate/weblate/trans/tests/test_files.py", line 175, in test_import_xliff
    self.assertContains(response, 'updated: 1')
  File "/home/travis/virtualenv/python3.6.3/lib/python3.6/site-packages/django/test/testcases.py", line 369, in assertContains
    self.assertTrue(real_count != 0, msg_prefix + "Couldn't find %s in response" % text_repr)
AssertionError: False is not true : Couldn't find 'updated: 1' in response

nijel · 2018-11-28T15:48:24Z

Is this still WIP or it should be ready for review? Anyay it at least needs rebase on current code (I guess introduction of xliff states caused the conflicts).

This should allow to translate units containing placeholders such as the one used by Angular i18n. PoXliff will not support placehoders, because it probably doesn't make sense to support XLIFF specific placeholders in a format that actually embed PO placeholders. Fixes WeblateOrg#490 Fixes WeblateOrg#1535 Signed-off-by: Adrien Crivelli <adrien.crivelli@gmail.com>

codeclimate · 2018-11-29T11:21:06Z

weblate/formats/ttkit.py

@@ -415,6 +417,29 @@ class XliffUnit(TTKitUnit):
        'new', 'needs-translation', 'needs-adaptation', 'needs-l10n'
    ))

+    @cached_property


Similar blocks of code found in 2 locations. Consider refactoring.

codeclimate · 2018-11-29T11:21:06Z

weblate/formats/ttkit.py

@@ -501,6 +526,19 @@ def mark_approved(self, value):
        if self.xliff_state:
            self.xliff_node.set('state', 'final' if value else 'translated')

+class PoXliffUnit(XliffUnit):


Expected 2 blank lines, found 1

PowerKiKi · 2018-11-29T12:50:32Z

This is not a WIP anymore and should be reviewed for merge. I rebased and squashed on master. Let me know if that's ok.

nijel · 2018-11-30T09:29:21Z

weblate/formats/ttkit.py

@@ -501,6 +526,19 @@ def mark_approved(self, value):
        if self.xliff_state:
            self.xliff_node.set('state', 'final' if value else 'translated')

+class PoXliffUnit(XliffUnit):


I'm not really sure this is needed. PoXliff is also used natively to get some of the gettext features to xliff, so disabling xliff placeholders might be not intended here.

nijel · 2018-11-30T09:30:26Z

Thanks, looks good, I'll merge this after releasing 3.3. I'd probably remove PoXliffUnit as that seems not wanted behavior to me.

Fixes #490 Fixes #1535 Signed-off-by: Michal Čihař <michal@cihar.com>

nijel · 2018-11-30T14:18:04Z

Merged, thanks for your contribution!

PowerKiKi · 2018-11-30T15:07:33Z

My pleasure

PowerKiKi force-pushed the xliff-placeholder-support branch from 607f635 to ab067ee Compare October 31, 2018 11:28

codeclimate bot reviewed Oct 31, 2018

View reviewed changes

PowerKiKi mentioned this pull request Nov 11, 2018

[i18n] plans angular/angular#16477

Closed

20 tasks

PowerKiKi force-pushed the xliff-placeholder-support branch from 4287828 to b186022 Compare November 12, 2018 04:51

WeblateOrg deleted a comment Nov 12, 2018

PowerKiKi changed the title ~~WIP Add support for XLIFF placeholders~~ Add support for XLIFF placeholders Nov 29, 2018

PowerKiKi force-pushed the xliff-placeholder-support branch from b186022 to 12252a9 Compare November 29, 2018 10:37

codeclimate bot reviewed Nov 29, 2018

View reviewed changes

nijel reviewed Nov 30, 2018

View reviewed changes

nijel self-assigned this Nov 30, 2018

nijel added this to the 3.4 milestone Nov 30, 2018

nijel merged commit 12252a9 into WeblateOrg:master Nov 30, 2018

nijel added a commit that referenced this pull request Nov 30, 2018

Add changelog for #2364

da876ac

Fixes #490 Fixes #1535 Signed-off-by: Michal Čihař <michal@cihar.com>

nijel mentioned this pull request Nov 30, 2018

xliff tags not deleted on pull #2011

Closed

nijel mentioned this pull request Feb 11, 2019

[XLIFF] ns0 littering output XML #2526

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for XLIFF placeholders #2364

Add support for XLIFF placeholders #2364

PowerKiKi commented Oct 31, 2018

codeclimate bot Oct 31, 2018

codeclimate bot Oct 31, 2018

codeclimate bot Oct 31, 2018

nijel commented Oct 31, 2018 •

edited

nijel commented Oct 31, 2018

codecov bot commented Nov 6, 2018 •

edited

PowerKiKi commented Nov 7, 2018

nijel commented Nov 9, 2018

nijel commented Nov 28, 2018

codeclimate bot Nov 29, 2018

codeclimate bot Nov 29, 2018

PowerKiKi commented Nov 29, 2018

nijel Nov 30, 2018

nijel commented Nov 30, 2018

nijel commented Nov 30, 2018

PowerKiKi commented Nov 30, 2018



		def string_to_rich(string):
		"""Transform a string containing XLIFF placeholders as XML into a rich content (StringElement)"""



		def rich_to_string(string_elements):
		"""Transform rich content (StringElement) into a string with placeholder kept as XML"""

Add support for XLIFF placeholders #2364

Add support for XLIFF placeholders #2364

Conversation

PowerKiKi commented Oct 31, 2018

codeclimate bot Oct 31, 2018

Choose a reason for hiding this comment

codeclimate bot Oct 31, 2018

Choose a reason for hiding this comment

codeclimate bot Oct 31, 2018

Choose a reason for hiding this comment

nijel commented Oct 31, 2018 • edited

nijel commented Oct 31, 2018

codecov bot commented Nov 6, 2018 • edited

Codecov Report

PowerKiKi commented Nov 7, 2018

nijel commented Nov 9, 2018

nijel commented Nov 28, 2018

codeclimate bot Nov 29, 2018

Choose a reason for hiding this comment

codeclimate bot Nov 29, 2018

Choose a reason for hiding this comment

PowerKiKi commented Nov 29, 2018

nijel Nov 30, 2018

Choose a reason for hiding this comment

nijel commented Nov 30, 2018

nijel commented Nov 30, 2018

PowerKiKi commented Nov 30, 2018

nijel commented Oct 31, 2018 •

edited

codecov bot commented Nov 6, 2018 •

edited