-
Notifications
You must be signed in to change notification settings - Fork 993
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
Fixes #36839 - Optimize apipie-dsl translate method #9866
Fixes #36839 - Optimize apipie-dsl translate method #9866
Conversation
164f47d
to
ec40f41
Compare
[test katello] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @ekohl, seems right, didn't test though yet.
Should we do the same for apipie-rails translate method?
ec40f41
to
30116da
Compare
I didn't test it either, but how about this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much cleaner, thanks!
AFAIU, with_locale
is proper (recommended) way instead of reseting with set_locale
, so functional-wise it should be the same.
config/initializers/apipie.rb
Outdated
FastGettext.set_locale(old_loc) | ||
trans | ||
end | ||
config.translate = ->(stc, loc) { str ? FastGettext.with_locale(loc) { _(str) } : nil } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config.translate = ->(stc, loc) { str ? FastGettext.with_locale(loc) { _(str) } : nil } | |
config.translate = ->(str, loc) { str ? FastGettext.with_locale(loc) { _(str) } : nil } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argh, I had that fixed on my laptop, then rebased on my desktop where I made the mistake in the first place ...
30116da
to
422ab27
Compare
I don't think Katello failures are related, but just in case... [test katello] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @ekohl, merging.
No description provided.