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 unescaped interpolated reserved key in help text and handle escaped interpolations in inconsistent interpolations check #553

Merged
merged 3 commits into from
May 7, 2024

Conversation

Bilka2
Copy link
Contributor

@Bilka2 Bilka2 commented Mar 12, 2024

This fixes #552 if and only if ruby-i18n/i18n#688 or an equivalent is merged for the i18n gem. Otherwise escaping the offending interpolations does not appease the reserved keyword check.

This PR also contains a workaround for another issue related to escaped interpolations (ruby-i18n/i18n#689), see the code comment.

I removed some unused translations that had interpolations in them, their usage was removed in c1d1312.

@edolix
Copy link

edolix commented Apr 16, 2024

Thanks @Bilka2! It would be nice to have this branch merged - are you going to review the failed tests? Otherwise i can start from your branch and open up another PR 🙏

@Bilka2
Copy link
Contributor Author

Bilka2 commented Apr 16, 2024

@edolix As per the PR description, the tests are failing because they need they need the PR in the i18n gem to be merged: ruby-i18n/i18n#688. Without that PR, escaping the variables does nothing to avoid the error.

Merging that PR is the plan for the i18n gem: ruby-i18n/i18n#689 (comment)

The only way to fix this here in i18n-tasks without the i18n PR would be to completely remove %{default} from the locale.

@edolix
Copy link

edolix commented Apr 16, 2024

Apologies for the noise here, i realised that right after submitting my comment 🙏

@Bilka2
Copy link
Contributor Author

Bilka2 commented May 6, 2024

The i18n gem v1.14.5 has been released with the needed fix to respect escaped interpolations. So this PR is now good to go.

@davidwessman
Copy link
Collaborator

@Bilka2 Could you fix the last lint errors?

@davidwessman davidwessman requested a review from glebm May 7, 2024 07:48
@Bilka2 Bilka2 force-pushed the escaped-interpolations branch 2 times, most recently from 7141a34 to 24c5b48 Compare May 7, 2024 08:49
@Bilka2
Copy link
Contributor Author

Bilka2 commented May 7, 2024

Done. Apologies for the force pushes, a testing commit ended up on the wrong branch.

@davidwessman
Copy link
Collaborator

Done. Apologies for the force pushes, a testing commit ended up on the wrong branch.

No worries, I appreciate it 🙂

Copy link
Collaborator

@davidwessman davidwessman left a comment

Choose a reason for hiding this comment

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

It looks good to me, but I'll let @glebm review as well.

lib/i18n/tasks/translators/google_translator.rb Outdated Show resolved Hide resolved
spec/google_translate_spec.rb Outdated Show resolved Hide resolved
spec/google_translate_spec.rb Outdated Show resolved Hide resolved
spec/google_translate_spec.rb Outdated Show resolved Hide resolved
lib/i18n/tasks/command/options/common.rb Show resolved Hide resolved
@davidwessman
Copy link
Collaborator

I extracted the rubocop fixes to #564 and rebased this branch

@davidwessman
Copy link
Collaborator

I'll merge it to main so we can get the tests running and can keep on iterating. Well done with the upstream change @Bilka2 !

@davidwessman davidwessman merged commit dffd911 into glebm:main May 7, 2024
7 checks passed
@Bilka2 Bilka2 deleted the escaped-interpolations branch May 7, 2024 19:20
@mjankowski
Copy link

Thanks for merging this - will there be version bump soon, or should we point to repo for now to get this change?

@davidwessman
Copy link
Collaborator

Thanks for merging this - will there be version bump soon, or should we point to repo for now to get this change?

Please try it out in your repo, then we can cut a release faster.

@glebm is the one doing releases.

@tagliala
Copy link
Contributor

tagliala commented May 9, 2024

Please try it out in your repo, then we can cut a release faster.

Hi, I'm testing main branch with I18n 1.18.5 and it is working fine

@mjankowski
Copy link

Please try it out in your repo, then we can cut a release faster.

Sure thing. I updated a PR branch to point to the repo - mastodon/mastodon#30198 - and it both a) resolved the previous errors we were seeing that this change fixed, b) had a nice side effect of the fixed escape logic helping us find a place where we had a mistaken interpolation escape, c) as far as I can tell, at least via CI runs and some local checks, everything else is working correctly as before.

@glebm
Copy link
Owner

glebm commented May 10, 2024

Thanks for testing @mjankowski and @tagliala! Released v1.0.14

Thanks everyone!

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.

Interaction between add-missing and i18n gem reserved keys check?
6 participants