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

22041 when checking for "inconsistent interpolations" also check for "unrestored interpolations" #4

Merged

Conversation

barak-neeman
Copy link

"unrestored interpolations" is just a fancy name for translation texts that have '!!!!!' in them - the new restore interpolations method outputs !!!!! when it fails to restore correctly

References bookingexperts/support#22041

Copy link

@tom-brouwer-bex tom-brouwer-bex left a comment

Choose a reason for hiding this comment

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

Looks good overall, just one minor comment about code style.

Additionally however, shouldn't we also include this check at translation time? I think this is explicitly where the 'raise_interpolation_error' function is for: To already raise these errors during translation, rather than in a check afterwards.

locales.each do |locale|
data[locale].key_values.each do |key, value|
next unless value.is_a?(String)
next unless value.index('!!!!!')

Choose a reason for hiding this comment

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

Any reason not to use #include? ? It indicates the intent better I think

Choose a reason for hiding this comment

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

@barak-neeman Can you add this change, before I do my next review?

Copy link
Author

Choose a reason for hiding this comment

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

@barak-neeman Can you add this change, before I do my next review?

@tom-brouwer-bex my bad, I amended the commit without any changes... It's done now.

@barak-neeman barak-neeman force-pushed the 22041-chore-final-round-of-fixing-i18n-interpolations branch from 198684c to 3c278ec Compare August 13, 2024 13:05
@barak-neeman
Copy link
Author

Additionally however, shouldn't we also include this check at translation time?

Yes when the restoration is really bad, but that is rare and I don't really know how to detect a really bad restoration in code (no idea what they would look like).

The usual restore failure would be DeepL gobbling one or two characters around the tags, and for those cases it's quite useful to let the translation finish. It's helpful for improving the restore_interpolations method, and it's helpful for manually fixing the translated text.

The check here is just to prevent it from going into prod.

…"unrestored interpolations" (which is just a fancy name for translation texts that have '!!!!!' in them - the new restore interpolations method outputs !!!!! when it fails to restore correctly)

References bookingexperts/support#22041
@barak-neeman barak-neeman force-pushed the 22041-chore-final-round-of-fixing-i18n-interpolations branch from 3c278ec to 5279b0f Compare August 13, 2024 13:50
Copy link

@tom-brouwer-bex tom-brouwer-bex left a comment

Choose a reason for hiding this comment

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

LGTM

@barak-neeman barak-neeman merged commit d30e6bf into main Aug 14, 2024
8 checks passed
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.

2 participants