-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
chore(i18n): Translated charts and filters into Russian #29377
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #29377 +/- ##
===========================================
+ Coverage 60.48% 83.70% +23.21%
===========================================
Files 1931 521 -1410
Lines 76236 37492 -38744
Branches 8568 0 -8568
===========================================
- Hits 46114 31381 -14733
+ Misses 28017 6111 -21906
+ Partials 2105 0 -2105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@gooroodev please review |
🤔 For now, this change is too large for me to review. Consider splitting it into smaller parts. |
@gooroodev Could you please consider reviewing this following commit history? I've tried to make the edits atomic. I'm afraid that splitting this into several PRs would not help reviewing because in order to provide translations for new strings we have to run babel scripts. And they shake up all the language files, resulting in a huge diff. |
🤔 For now, this change is too large for me to review. Consider splitting it into smaller parts. |
I tried to have Claude (AI) review this, but it's too large. Even the smallest of the individual files' diffs was too large :P |
@rusackas, in this case I need your advice on how I should proceed. In accordance with the comment by @mistercrunch, I could try to get rid of the effects of
I believe the diff for messages.po will still be huge because of strings rearrangement. But would it somehow help to review the translation? Anyway, I believe these commands would still be required to run outside of this PR: ./scripts/translations/babel_update.sh
pybabel update -i superset/translations/messages.pot -d superset/translations --ignore-obsolete |
I think the issue is while we know (actually I'm not 100% on this, but maybe +95%) that rebuilding Knowing this, in theory that means that if/when there are many competing PRs with an older base, the first one to merge could make it a significant burden on the other ones as the diff/merge conflict doesn't actually make sense and can't be resolved line-by-line. I think no-one should ever try to resolve a merge conflict in those
Now if there were 2 big PRs open on the same language file, that wouldn't work and we'd have to get more creative as to how to merge the 2. Unlikely but a bit of a headache if/when it does. We can fix that with a bit of coordination between people who care about the same language. About translation PRs in general, personally as a review I'd prefer if there were 2 types of PR:
|
About this particular PR, if you kept just that one file for Russian file I'd feel better about approving/merging it. The issue being as reviewers we cannot really validate that this PR isn't removing some other new translations in some other language. If you did everything in the right order it should be ok, but we don't know this for a fact. I think there has been some issues of translations lost in the shuffle in the past (I'm not 100% on that, but pretty sure it did happen before), and if you keep only your one po file this PR is safer to merge. Some greater truths here:
|
…r selector, placeholders for dashboard filters
Thank you for the guidance. I've excluded all the changes except those done to the That Python-Integration test has failed again. Could you please take a look? |
Re-running that test. Hopefully it's just flaky. Thank you for simplifying the PR! |
Ok, here's some AI-generated feedback. Let me know your thoughts :)
Other than that, Claude thought you did an outstanding job :D Let me know if you think it's right or not on any of the above. |
Thank you, this feedback is very useful. I'll try to locate these strings in the UI and figure out how to deal with them. Let me convert this PR to draft, so it won't get accidentally merged while I'm reviewing these suggestions. |
I've reviewed the feedback and made some adjustments to the translation. You can find the breakdown below.
Funnel Chart. I felt like that literal translation is way too... literal. My thought process was that each segment of the funnel is some part of total. That part is represented with percentage value. The key difference of the selector values is not how we calculate percentages themselves, it's what we take as a base to get the share from. That's why I have picked this adaptation.
IIRC, originally it was "2/98 перцентиль" (singular in Russian as well). I haven't found this particular word in dictionaries (some people say these two versions are equivalent). Changed it because the original one felt too closely borrowed from the English counterpart and Russian Wikipedia page refers to this term as "процентиль". Furthermore, "percent" in Russian is "процент" and there is no apparent reason why would the child word should change its letter order.
I think this one refers to datetime formatting dropdown. The suggested translation is too long for the UI element and doesn't fit well into the context.
I'm not sure why the AI highlights this as a potential problem. These strings look alright to me, both in Poedit and in the UI itself.
I think this one was picked for
Good point, I've missed this fuzzy and some other strings for the histogram chart as well. I tend to use more specialized term for this entity when it comes to a histogram, "Бины". Will fix.
Quick search indicates that this one is used in legacy charts that are out of scope of this PR.
I can't find this particular string in the file.
That was exactly as how I have translated these strings. But I've noticed I have forgot to remove full stops at their ends. Update incoming :)
Again, "Расположение меток листьев" was my version.
I agree that these ones should be updated, but they are related to
Not sure why the AI has picked the old version of the translation, but I have interpreted this as "Падение" and its counterpart as "Рост". It looks good on waterfall charts.
Looks like this one is related to
I think that the accompanying description could use some improvements indeed, but it feels natural in its current state.
"Отображение" is how "Display" string is currently translated.
Will fix.
In my translation I've used "Формат графа" because of its brevity and placed "Способ отрисовки графа" into the tooltip. Just tried the longer version and it looked fine. I'll switch to the suggested variant.
Agreed.
Looks like that comment is related to a fuzzy string for
Yeah, I'll change that one.
"Loading..." is currently translated as "Загрузка..."
Missed that one. It's very tough though. There is no close short analog that could describe a "main" value as a value of an indicator for current period. Moreover, when I provide any translation for this string, no matter how short it is, the table just breaks. The values in the corresponding column just disappear. I'll provide the most basic viable translation "Значение" for this string and mark it as fuzzy for now. |
Now that this is merged I ran a global .po update here #29476 |
SUMMARY
I have checked and fixed Russian translations for the components of the chart editor that are related to up-to-date charts. These are the ones affected:
Any charts that were marked as Legacy or Deprecated were left out of scope.
In addition, I have noticed that #28637 introduced "Current" option to dashboard filters. So I have extracted new strings and translated them as well. This is the reason why other i18n files are affected.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION