Serve three languages instead of shipping ten and serving one - #119
Merged
Conversation
The site carried ten locale files and rendered every request in English.
Multilang's `before_action :set_locale` was commented out and the
locale switcher was commented out of the layout, so `?locale=ru` did
nothing and no visitor ever saw a translated page.
That was deliberate, not rot: the project did not have the people to
keep ten translations in step with the English copy, and a stale
translation of the flashing instructions is worse than an English one.
The maintainers' decision now is three -- English as the source,
Russian and Chinese for the two largest communities. Translating got
cheap; keeping translations honest through every later edit did not,
which is why the answer is three and not ten again.
- available_locales and i18n-tasks both list en, ru, zh.
- The 35 locale files for de, es, fa, fr, it, pl, pt are removed.
- ru and zh were four keys short of English each; those are written.
i18n-tasks missing goes 37 -> 4 and unused 307 -> 103. The four that
remain are older: two keys referenced from code that exist in no
locale at all, and Russian plural forms for errors.messages.not_saved.
Three defects had to be fixed before switching it on, all of them
invisible while it was off.
`browser_locale` scanned `/[a-z]{2}(?=;)/`, which only matches a tag
followed by a q-value. The first entry of an Accept-Language header
does not carry one, so the visitor's top preference was the single
entry the scan could never see: `ru,en;q=0.9` answered `en`.
LOCALES fed the switcher while available_locales fed everything else,
and they disagreed -- LOCALES listed Japanese, which has no locale file
and was never available. The switcher would have offered a language the
app cannot render. A test now holds the two together.
`locales_for_select` looked up `t("locales.#{l}")`, and no locale file
has `locales.*` keys, so every entry would have come back as a
translation-missing span. It uses LOCALES, like the switcher always has.
set_locale is an around_action wrapping I18n.with_locale rather than a
before_action assigning I18n.locale. I18n.locale is per-thread and
nothing resets it at the end of a request.
Three tests elsewhere used I18n.with_locale to reach Russian, because
the parameter did nothing. They use ?locale=ru now -- the path a
visitor actually takes.
221 runs, 828 assertions. Restoring the old scan fails 2; switching
set_locale back off fails 4; putting Japanese back fails 2. 10 rubocop
offences across the touched files, against 15 on master.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YFg9PRy2T6cr983S8gran5
PR Summary by QodoEnable maintained three-language localization
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
These were flagged and not fixed in the commit before this one. With
three languages actually being served they stop being bookkeeping.
`pages.qr_code.title` was never a missing translation. The controller
asks for it; the key is `pages.qr_code_generator.title`, and has been
all along. The QR generator's browser tab has been reading "translation
missing: en.pages.qr_code.title" in every language, English included.
`site.snapshot.view_heif` existed only as an inline English default in
the view, so it rendered as English on a Russian page and i18n-tasks
could not tell it was untranslated. It is a key in all three locales now
and the default is gone.
Russian `errors.messages.not_saved` had `one` and `other`. Russian has
four forms, and adding `few` and `many` to the file changes nothing on
its own: I18n's default pluralizer knows two, so every count from 2 up
kept taking `other` -- "5 ошибки" where Russian wants "5 ошибок", "21
ошибки" where it wants "21 ошибка". lib/locale/plurals.rb carries the
CLDR rule and config/initializers/locale.rb mixes in
I18n::Backend::Pluralization, without which the rule is read and
ignored. rails-i18n would be the answer for ten locales; for three, one
of which needs no rule, it is more than the problem is worth.
Checked across the boundaries the rule turns on:
1 ошибка 2 ошибки 5 ошибок 11 ошибок
21 ошибка 22 ошибки 25 ошибок 101 ошибка
`i18n-tasks missing` is 0, from 37 on master.
Five tests. One of them passed against the unfixed code on the first
attempt: it looked for /translation missing/ and I18n writes
"Translation missing". Case-insensitive now, and it fails when the
Russian key is removed.
225 runs, 853 assertions. Restoring the typo fails 1, removing the
pluralization include fails 1, removing the Russian key fails 1.
13 rubocop offences across the touched files, against 14 on master.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YFg9PRy2T6cr983S8gran5
Collaborator
Author
|
/review |
|
Code review by qodo was updated up to the latest commit 7fdbcb4 |
Qodo's review of the two commits before this. Four findings, all real, and all of them things that were harmless only while the switcher was commented out of the layout. The icon was a hardcoded <img src="/assets/translate.svg">. Production serves fingerprinted assets with compile off, so that is a 404 -- and the switcher is in the layout, so it is a 404 on every page of the site. IconsHelper#icon_language has rendered the same file through image_tag all along. Every link was a bare "?locale=xx", which replaces the query string rather than editing it. Switching language on a paginated list lost the page, and on a permanent link to a camera configuration -- the thing three commits of this session were spent making work -- it lost the configuration. Both of those come from locale_switcher building HTML by hand in a controller concern, where image_tag and the request are out of reach. It is a partial now, which is also where a reviewer would look for it. While rewriting: aria-labelledby said "dropdownLanguage" and the button was id="dropsownLanguage", so it labelled nothing. The Accept-Language parser kept q=0 entries. q=0 means "not acceptable" (RFC 9110 12.4.2), so `ru;q=0,de;q=0.9` answered `ru` -- a language the visitor had explicitly ruled out -- because `de` is not served. And it matched ';q=' as a literal, where the grammar allows whitespace around the separator and either case of the name. `en; q=0.1,ru;q=0.9` ranked English at the default 1 and picked it, which is the reverse of what the header says. 231 runs, 873 assertions. Six new tests, each checked by reverting its fix: q=0 fails 1, the literal ';q=' fails 2, the hardcoded asset and the bare query string fail 2. 9 rubocop offences across the touched files against 14 on master -- the hand-built HTML took several with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YFg9PRy2T6cr983S8gran5
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The site carried ten locale files and rendered every request in English.
Multilang'sbefore_action :set_localewas commented out atmultilang.rb:21and the switcher was commented out of the layout, so
?locale=rudid nothingand no visitor ever saw a translated page.
That was deliberate, not rot — the project did not have the people to keep ten
translations in step with the English copy, and a stale translation of the
flashing instructions is worse than an English one. The maintainers' decision
now is three: English as the source, Russian and Chinese for the two
largest communities.
The change
available_localesandconfig/i18n-tasks.ymlboth listen, ru, zh.i18n-tasks missinggoes from 37 to 0;unusedfrom 307 to 103.Seven things that had to be fixed before switching it on
Every one was harmless only because the switcher was commented out.
Browser detection ignored the visitor's first choice.
browser_localescanned
/[a-z]{2}(?=;)/, which only matches a tag followed by a q-value — andthe first entry of an
Accept-Languageheader does not carry one.ru,en;q=0.9answered
enfor a Russian speaker.q=0was treated as a preference. It means "not acceptable" (RFC 911012.4.2), so
ru;q=0,de;q=0.9answeredru— a language the visitor hadexplicitly ruled out — because
deis not served.;q=was matched as a literal. The grammar allows whitespace around theseparator and either case of the name, so
en; q=0.1,ru;q=0.9ranked English atthe default 1 and picked it.
The switcher icon bypassed the asset pipeline. A hardcoded
<img src="/assets/translate.svg">, against a production config withassets.compile = falseand fingerprinted assets — a 404 on every page,since the switcher is in the layout.
IconsHelper#icon_languagehad rendered thesame file correctly all along.
The switcher discarded the query string. Every link was a bare
?locale=xx,which replaces rather than edits. Switching language on a paginated list lost the
page — and on a
Camera#permalinkURL it lost the whole camera configuration.LOCALESandavailable_localesdisagreed.LOCALESlisted Japanese: nolocale file, never available. The switcher would have offered a language the app
cannot render. A test now holds the two together.
locales_for_selectwas broken. It looked upt("locales.#{l}")and nolocale file has
locales.*keys, so every entry would have been atranslation-missing span.
The first two switcher bugs came from
locale_switcherbuilding HTML by handinside a controller concern, where
image_tagand the request are out of reach.It is a partial now. While rewriting it:
aria-labelledbysaiddropdownLanguagewhile the button wasid="dropsownLanguage".And one design change:
set_localeis anaround_actionwrappingI18n.with_localerather than abefore_actionassigningI18n.locale, whichis per-thread with nothing resetting it at request end.
Three older i18n gaps, closed
pages.qr_code.titlewas a typo, not a missing translation — the key ispages.qr_code_generator.title. That page's browser tab has been readingtranslation missing: en.pages.qr_code.titlein every language.site.snapshot.view_heifexisted only as an inline English default, so itread as English on a Russian page and
i18n-taskscould not see it.few/manyto the file alone does nothing —there is no
rails-i18n, so I18n's default pluralizer knows two forms andevery count from 2 up kept taking
other.lib/locale/plurals.rbcarries theCLDR rule and the initializer mixes in
I18n::Backend::Pluralization:Verification
bin/rails test— 231 runs, 873 assertions, 0 failures (208 on master).Every fix checked by reverting it: old scan 2,
set_localeoff 4, Japanese back2,
q=0kept 1, literal;q=2, hardcoded asset + bare query string 2, thetitle typo 1, no pluralization include 1, Russian key removed 1.
9 rubocop offences across the touched files, against 14 on master — the
hand-built HTML took several with it.
i18n-tasks missingis 0.Read in all three:
🤖 Generated with Claude Code
https://claude.ai/code/session_01YFg9PRy2T6cr983S8gran5