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

fix: Remove annotation Fuzzy to get french translation #26010

Merged

Conversation

aehanno
Copy link
Contributor

@aehanno aehanno commented Nov 16, 2023

Fix this pb : #25728

@aehanno aehanno changed the title Without the capital letter, you can't find the translation fix: Without the capital letter, you can't find the translation Nov 16, 2023
@sfirke
Copy link
Member

sfirke commented Nov 17, 2023

It looks like all of the other languages have lower case (example), which matches the base set for translation. French has uppercase and it's not matching. Do you agree the French should be changed to lowercase? I worry that this PR would fix French and break all of the other languages.

Copy link
Member

@sfirke sfirke left a comment

Choose a reason for hiding this comment

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

See my comment in the PR conversation, I think the proper fix would be to make the French translation values lower case, not the base ones capitalized.

Add translation from script po2json
@pull-request-size pull-request-size bot added size/M and removed size/S labels Nov 17, 2023
@aehanno
Copy link
Contributor Author

aehanno commented Nov 17, 2023

Hey @sfirke, yes I just saw that, I saw thaht the problem is the annatation #, fuzzy in po file
Do you know why we have that and is it's correct if I just remove this annotation ?

@sfirke
Copy link
Member

sfirke commented Nov 17, 2023

I'm unfamiliar with translations in general. Sounds like the fuzzy there means, attention needed. What problem is the fuzzy comment causing?

@sfirke
Copy link
Member

sfirke commented Nov 17, 2023

Also thanks for updating the French strings to match the base set, it looks good now.

Fix translation
@aehanno
Copy link
Contributor Author

aehanno commented Nov 17, 2023

I'm unfamiliar with translations in general. Sounds like the fuzzy there means, attention needed. What problem is the fuzzy comment causing?

The problem with the fuzzy, it's with this annotation when I launch the script po2json.sh to have my translation. All the string with this annotation are not taken into account.
So they don't appear in the json file
So the translation doesn't appear on the interface either

@sfirke
Copy link
Member

sfirke commented Nov 17, 2023

Ah okay. I don't see "fuzzy" appearing in the codebase anywhere so I'd say go ahead and delete that. If you can test locally that all the desired translations are appearing, that works for me.

@@ -4591,18 +4618,10 @@
"y: values are normalized within each row": [""],
"year": ["année"],
"zoom area": [""],
"No matching records found": ["Aucun résultat trouvé"],
Copy link
Member

Choose a reason for hiding this comment

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

Did you replace this string? I see it deleted here but no other mention of "no matching records found"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just annotation fuzzy on this string, I remove it

Copy link
Member

Choose a reason for hiding this comment

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

I don't follow, sorry. Will the string "No matching records found" still be translated?

Fix translation
Copy link
Member

@sfirke sfirke left a comment

Choose a reason for hiding this comment

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

LGTM

@sfirke
Copy link
Member

sfirke commented Nov 17, 2023

This is my first time working with translations - anyone who has more experience want to review + merge?

Fix translation
@pull-request-size pull-request-size bot added size/L and removed size/M labels Nov 17, 2023
Copy link

codecov bot commented Nov 17, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5942d8b) 66.90% compared to head (ed2affd) 69.06%.
Report is 8 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #26010      +/-   ##
==========================================
+ Coverage   66.90%   69.06%   +2.16%     
==========================================
  Files        1938     1938              
  Lines       75837    75858      +21     
  Branches     8427     8427              
==========================================
+ Hits        50736    52389    +1653     
+ Misses      22931    21299    -1632     
  Partials     2170     2170              
Flag Coverage Δ
hive 53.66% <ø> (?)
mysql 78.16% <ø> (-0.04%) ⬇️
postgres 78.26% <ø> (-0.04%) ⬇️
presto 53.61% <ø> (?)
python 82.91% <ø> (+4.48%) ⬆️
sqlite 76.91% <ø> (-0.04%) ⬇️
unit 55.73% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aehanno aehanno changed the title fix: Without the capital letter, you can't find the translation fix: Remove annotation Fuzzy to get french translation Nov 17, 2023
@aehanno
Copy link
Contributor Author

aehanno commented Nov 20, 2023

@sfirke, Hey thanks for the review.
Do you know if it can be merge in the 3.0.2 release
It will be so great

Copy link
Member

@sfirke sfirke left a comment

Choose a reason for hiding this comment

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

Did you mean to translate "JSON" to "YAML"?

superset/translations/fr/LC_MESSAGES/messages.json Outdated Show resolved Hide resolved
superset/translations/fr/LC_MESSAGES/messages.po Outdated Show resolved Hide resolved
@@ -4591,18 +4618,10 @@
"y: values are normalized within each row": [""],
"year": ["année"],
"zoom area": [""],
"No matching records found": ["Aucun résultat trouvé"],
Copy link
Member

Choose a reason for hiding this comment

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

I don't follow, sorry. Will the string "No matching records found" still be translated?

@aehanno
Copy link
Contributor Author

aehanno commented Nov 20, 2023

for the quesiton : #26010 (comment)

The translation is present line 2254

Correction translation
Copy link
Member

@sfirke sfirke left a comment

Choose a reason for hiding this comment

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

Merci pour les améliorations! (Looks good to me 😜)

@sfirke sfirke added the merge-if-green If approved and tests are green, please go ahead and merge it for me label Nov 20, 2023
Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

LGTM!

@rusackas rusackas merged commit 25a737e into apache:master Nov 21, 2023
33 checks passed
@michael-s-molina michael-s-molina added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Nov 21, 2023
michael-s-molina pushed a commit that referenced this pull request Dec 4, 2023
josedev-union pushed a commit to Ortege-xyz/studio that referenced this pull request Jan 22, 2024
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@mistercrunch mistercrunch added 🍒 3.0.3 🍒 3.0.4 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024
sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels merge-if-green If approved and tests are green, please go ahead and merge it for me size/L v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch 🍒 3.0.3 🍒 3.0.4 🚢 3.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants