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

[Translation] Skip state=needs-translation entries only when source == target #54564

Merged
merged 1 commit into from
Apr 12, 2024

Conversation

nicolas-grekas
Copy link
Member

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #54541
License MIT

@nicolas-grekas nicolas-grekas changed the title [Translation] Ignore state=needs-translation only when source == target [Translation] Skip state=needs-translation entries only when source == target Apr 11, 2024
@nicolas-grekas nicolas-grekas added this to the 5.4 milestone Apr 11, 2024
@kevinpapst
Copy link

kevinpapst commented Apr 11, 2024

After managing translations in over 30 languages in a project with thousands of translations I can tell you, there are all kind of weird combinations ... What happens if you have such a combination (this is a stupid example I know, but you get the point).

English:

      <trans-unit id="5">
        <source>Save</source>
        <target state="needs-translation">Persist</target>
      </trans-unit>

French:

      <trans-unit id="5">
        <source>Save</source>
        <target state="needs-translation">Save</target>
      </trans-unit>

Would French now show Save or Persist? Because previously it would still show Save, which is what I would expect as well. IMO the needs-translation state is only for the translator tooling.

May I ask: what was your initial idea for that change? Did you plan to reduce the cache size? Which problem are we trying to solve with ignoring the translations?

@nicolas-grekas
Copy link
Member Author

It will show "Persist" if the fallback language is "en". Possibly "Save" if the key is used as a fallback instead.
That's fine anyway to me :)

@kevinpapst
Copy link

Hm, I expected that answer.

That's fine anyway to me :)

Yep. To me it is not 😁 and it is still breaking behavior.
Question is: what is the best behavior for the framework.
Was there ever a complaint about the previous situation?

Currently hundreds of translations are missing throughout two dozen locales and this PR will only fix it partially.

If you skip existing translations based on a flag (that is to me only meant for the tooling) and some assumptions, then I can't use the framework reader any more. I cannot remove the flag from the XLIFF, because it is needed for the translators.
From my POV the software should show what is inside the translation files. The key is not missing, it is there.
And you remove it from the catalogue to solve which problem?
Currently you assume that it is not needed, but my example shows that this might lead to wrong/unexpected results.

If you insist one keeping the behavior, can we make it configurable?

@nicolas-grekas
Copy link
Member Author

The change has been made to allow filling in xliff files with missing keys, while still allow a fallback to the preferred language. We use this since that recent PR to make it easier for translators to spot what's missing.
That's a nice workflow we want to support (and we use it already for Symfony itself.)
I thought about using some x- foo state.
I'm not in favor of yet another flag to configure.
What would be your proposal?

@kevinpapst
Copy link

kevinpapst commented Apr 11, 2024

Thanks for not rejecting my arguments immediately 😄

Maybe this is an issue how Weblate handles its translations. I just want to repeat that to explain the problem I have and I doubt that I am the only one using that: if the base translation changes in Weblate, all other languages are flagged as "needs-translation". It does not mean they are suddenly invalid.

Don't get me wrong, this PR will already solve > 95% of my current issues, so I would approve it no matter what.
But to solve all possible issues I would like to be able to configure if certain states should be skipped from loading into the catalogue. Because the <source> is not necessarily what is shown in the fallback language.

I checked the states in the XLIFF RFC and there is also "needs-review" and some other review states, but no matter which one we choose, it might not match the workflow or the used tooling.
So I would propose to make the "skip" states configurable. Probably allow to pass in a list of states?
But I also understand that adding more options always complicates things... so I don't mind if you ignore my entire rumbling here 😁

Copy link

@kevinpapst kevinpapst left a comment

Choose a reason for hiding this comment

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

Tested, works, thank you!

@nicolas-grekas nicolas-grekas merged commit dc7b54d into symfony:5.4 Apr 12, 2024
9 of 12 checks passed
@nicolas-grekas nicolas-grekas deleted the tr-fix branch April 19, 2024 15:42
This was referenced Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants