Refactor frontend and backend locale_controllers #4126
Conversation
Refactored by: - encapsulating the if statement to the new private method `locale_is_available?` for readability and context - making the `requested_locale` a symbol instead of mapping the `available_locales` to a string during `locale_is_available?` - moving setting of the `session`'s' locale line down one to be consistent with the `backend`'s `locale_controller`
Refactored by: - renaming `locale` to `requested_locale` - encapsulating the if statement to the new private method `locale_is_available?` for readability and context - removing unnecessary `respond_to` and `format.json`
| respond_to do |format| | ||
| format.json { render json: { locale: I18n.locale }, status: 404 } | ||
| end | ||
| render json: { locale: I18n.locale }, status: 404 |
There was a problem hiding this comment.
Rails/HttpStatus: Prefer :not_found over 404 to define HTTP status code.
There was a problem hiding this comment.
From what I have seen in Solidus, is that only or mainly status codes are used rather than the symbol equivalents. Should this style be turned off or does the core team actually prefer the symbols?
There was a problem hiding this comment.
I'd gradually refactor the codebase following the linter rules when some lines are changed. This way we keep improving the codebase without big refactors that can compromise git history. The trade-off here is not being consistent, which I don't like either. I'd go with the first, if we have a linter setup with Hound (only checks lines changed) we should use it otherwise it doesn't make sense to keep it.
There was a problem hiding this comment.
Great to know for the future! Thank you 👍
| respond_to do |format| | ||
| format.json { render json: { locale: I18n.locale }, status: 404 } | ||
| end | ||
| render json: { locale: I18n.locale }, status: 404 |
There was a problem hiding this comment.
I'd gradually refactor the codebase following the linter rules when some lines are changed. This way we keep improving the codebase without big refactors that can compromise git history. The trade-off here is not being consistent, which I don't like either. I'd go with the first, if we have a linter setup with Hound (only checks lines changed) we should use it otherwise it doesn't make sense to keep it.
The goal of these changes were to refactor where possible, improve readability and make the
backendandfrontendlocale_controllers more consistent with each other.locale_is_available?for readability and contextrequested_localea symbol instead of mapping theavailable_localesto a string duringlocale_is_available?session's' locale line down one to beconsistent with the
backend'slocale_controllerlocaletorequested_localelocale_is_available?for readability and contextrespond_toandformat.jsonChecklist: