21941 replaces the <i18n> interpolation tag in i18n-tasks for our cus… #3
Conversation
a5c1843 to
97a86ff
Compare
There was a problem hiding this comment.
Very nice work 👍 Feels a bit hacky that we have to do Deepl's job, but I couldn't find any corner cases that will fail.
Maybe we can also open a ticket for Deepl to fix this sometime in the future
| LETTER_SUBS = %w[RYX QFN VLB XOG DWP ZMQ JZQ WVS LRX HPM].freeze | ||
| NUM_SUBS = %w[17 19 23 29 31 37 41 43 47 53].freeze | ||
|
|
||
| def key_substitution(stache, index) |
There was a problem hiding this comment.
What's a stache?
I could only find this: https://en.wiktionary.org/wiki/stache . Hope it's not the 'pedo-stache' from that page you're reverring to? 😄
There was a problem hiding this comment.
@tom-brouwer-bex Just an old name for inlining variables in an html string.
Ruby calls it interpolation, which is a misuse of the mathematical term.
How about handle="" to be a bit more formal? (stache - moustache - handlebars - handle)
There was a problem hiding this comment.
'handle', or perhaps 'key', since that's what the function is called. Also in the code below, in the gzip function, you increment the 'key_index' for every 'stache' in that case I'd expect it to be called 'key'.
However, in the docs (and also in your html tag) it's called 'variables' (e.g. see https://guides.rubyonrails.org/i18n.html#passing-variables-to-translations), and the feature is called 'variable interpolation', so i'd personally opt for a signature like 'substitute_variable(variable, index)' (or 'translation_var(iable)', if 'variable' is too generic), and then also use 'variable_index' in the code below.
There was a problem hiding this comment.
@tom-brouwer-bex I'm trying, perhaps not very well, to keep a distinction between a handle that is wrapped "{{ name }}" or "%{ name }" and a key that is just "name". Just plain index looks better to me. Code updated.
| LETTER_SUBS = %w[RYX QFN VLB XOG DWP ZMQ JZQ WVS LRX HPM].freeze | ||
| NUM_SUBS = %w[17 19 23 29 31 37 41 43 47 53].freeze | ||
|
|
||
| def key_substitution(stache, index) |
There was a problem hiding this comment.
Since the methods in this file are verbs, consider renaming to 'substitute_key' or similar, or better yet 'substitute_interpolation', since this is the terminology already used. I'd opt for either the verb, 'substitute_interpolation' (consistent with naming of the rest of the functions), or naming it after the output ('interpolation_substitute') rather than the process (interpolation_substitution)
spec/deepl_translate_spec.rb
Outdated
| @@ -92,4 +92,97 @@ | |||
| end | |||
| end | |||
| end | |||
|
|
|||
| # Don't expect deepl's answers to be exactly the same each run | |||
| describe 'in het Nederlands' do | |||
There was a problem hiding this comment.
Let's not use dutch in our source code 😉
spec/deepl_translate_spec.rb
Outdated
| let(:base_task) { I18n::Tasks::BaseTask.new } | ||
|
|
||
| before do | ||
| # this is a good place to go ENV['DEEPL_AUTH_KEY'] = '...' but do not commit it |
There was a problem hiding this comment.
Why not just set the env variable when running the tests?
spec/deepl_translate_spec.rb
Outdated
|
|
||
| it 'sings' do | ||
| german, english, spanish = | ||
| translate_dutch(verse: 'Ik zou zo graag een %{animal} kopen. Ik Zag %{count} beren %{food} smeren') |
There was a problem hiding this comment.
'Zag' should not be capitalized ⛏️
…tom <var> tag This allows DeepL to "read" the interpolation contents, which improves translation accuracy and also fixes the problem of interpolations getting pushed to the front of sentences Closes bookingexperts/monorepo#21941
97a86ff to
b135469
Compare
i18n-tasks normally send DeepL something like:
Taak "<i18n>%{todo}</i18n>" toegewezen aan <i18n>%{user}</i18n>
and gets back
<i18n>%{todo}</i18n>To-dos " " zugewiesen an <i18n>%{user}</i18n>
That mess is the reason why this PR is needed
In this PR we send a custom <var> tag like:
Taak "<var handle="%{todo}" sub="RYX">RYX</var>" toegewezen aan <var handle="%{user}" sub="QFN">QFN</var>
and get back
To-dos "<var handle="%{todo}" sub="RYX">RYX</var>" zugewiesen an <var handle="%{user}" sub="QFN">QFN</var>
XML attributes, handle and sub, aren't modified by DeepL, so we can rely on those to come back with the same values we sent, enabling us to undo the substitution.
Now we can allow DeepL to "read" the tags (whereas before they were ignored), and that makes for better translations, and solves the problem of interpolations getting pushed to the front.
What we get back isn't all sunshine and rainbows, though, DeepL will still occasionally mangle it. More details in the code.