-
Notifications
You must be signed in to change notification settings - Fork 1
[Task #1080] Add locale negotiation #52
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
Conversation
10e5b39
to
c7cc312
Compare
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.
Great contribution 🙇 , I'm interested to hear your thoughts on my suggestions.
|
||
def rescue_with_handler(*) | ||
# Must render error in valid locale | ||
valid_locale = locale_valid?(locale) ? locale : I18n.default_locale |
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.
I find it odd that we check locale validity here but don't do the same in #setup_locale
. Can you please elaborate the decision?
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.
This gem handles I18n::InvalidLocale
via rescue_from
in InfinumJsonApiSetup::JsonApi::ErrorHandling
:
rescue_from I18n::InvalidLocale do |e|
render_error(InfinumJsonApiSetup::Error::BadRequest.new(message: e.to_s))
end
if we add localised error rendering here and render an invalid locale we get an error again.
However, in the latest commit this mixin will handle the invalid locale gracefully and respond with a HTTP 400 (default) or fallback to I18n.default_locale
. Thus, the validity check was also removed from error rendering.
accept_language = request.env['HTTP_ACCEPT_LANGUAGE'] | ||
return unless accept_language | ||
|
||
accept_language.scan(/^[a-z]{2}/).first |
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.
Can you check out https://github.com/cyril/accept_language.rb and see if it makes sense to integrate that dependency as well?
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.
I've considered this gem but I took the option of not adding another dependency, instead using the simpler approach similar to what we have elsewhere.
If we're cool with another dependency for a more feature-full parser I'm all for it 👍🏻
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.
accept_language
seems lightweight, only other dependency is from standard library practically, bigdecimal
. It gets a pass from me but I'm leaving the decision to you.
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.
accept_language
was added
c7cc312
to
13ae6e7
Compare
13ae6e7
to
1aeb32e
Compare
@@ -1 +1 @@ | |||
rails.7.0.gemfile No newline at end of file | |||
rails.7.1.gemfile No newline at end of file |
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.
Symlink was broken
@@ -1 +1 @@ | |||
rails.7.0.gemfile.lock No newline at end of file | |||
rails.7.1.gemfile.lock No newline at end of file |
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.
Same as above
Related issue: #1080