locize / xliff

xliff2js and js2xliff converter xliff utils
MIT License
80 stars 37 forks source link

Add support for nested `<group>` tags #29

Closed probertson closed 4 years ago

probertson commented 4 years ago

A <group> can contain translation units (1.2: <trans-unit>, 2.0: <unit>) and it can also contain other <group>s.

There was no support for <group> in the XLIFF 2.0 code so I added that as well.

This resolves #28

Checklist

adrai commented 4 years ago

Wouldn't it be better to enable this feature only with a flag? I didn't check, but wouldn't converting a such a file => https://github.com/locize/xliff/pull/29/files#diff-b44d024de9f8f4635e08e69b29aefb6e to js, be breaking, because "group1" could be interpreted as a new key?

adrai commented 4 years ago

Probably targetOfjs and sourceOfjs needs to be checked too...

probertson commented 4 years ago

I didn't check, but wouldn't converting a such a file => https://github.com/locize/xliff/pull/29/files#diff-b44d024de9f8f4635e08e69b29aefb6e to js, be breaking, because "group1" could be interpreted as a new key?

@adrai The tests pass, including the ones for those files. (I added the tests first and then added the code to make them work.) It distinguishes <unit> from <group> (xml -> js) by checking the name of the tag, and (js -> xml) by checking whether the object (translation unit or group) has a groupChildren property.

Maybe I'm misunderstanding your question though. And it's very possible that I'm missing something. I initially thought this would require a breaking change to the JS object structure, but I just reused the structure that was already in use for groups and it seemed to work.

adrai commented 4 years ago

ok, will merge asap

probertson commented 4 years ago

Oh, I meant to tag @amadare42 on this to double check that this matches what you expect

adrai commented 4 years ago

released with v5.1.0