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

Add new Rails/I18nLazyLookup cop #326

Merged
merged 1 commit into from
Jan 25, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/new_add_new_i18n_lazy_lookup_cop.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#326](https://github.com/rubocop/rubocop-rails/pull/326): Add new `Rails/I18nLazyLookup` cop. ([@fatkodima][])
9 changes: 9 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,15 @@ Rails/HttpStatus:
- numeric
- symbolic

Rails/I18nLazyLookup:
Description: 'Checks for places where I18n "lazy" lookup can be used.'
StyleGuide: 'https://rails.rubystyle.guide/#lazy-lookup'
fatkodima marked this conversation as resolved.
Show resolved Hide resolved
Reference: 'https://guides.rubyonrails.org/i18n.html#lazy-lookup'
Enabled: pending
VersionAdded: '<<next>>'
Include:
- 'controllers/**/*'

Rails/I18nLocaleAssignment:
Description: 'Prefer the usage of `I18n.with_locale` instead of manually updating `I18n.locale` value.'
Enabled: 'pending'
Expand Down
94 changes: 94 additions & 0 deletions lib/rubocop/cop/rails/i18n_lazy_lookup.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Rails
# This cop checks for places where I18n "lazy" lookup can be used.
#
# @example
# # en.yml
# # en:
# # books:
# # create:
# # success: Book created!
#
# # bad
# class BooksController < ApplicationController
# def create
# # ...
# redirect_to books_url, notice: t('books.create.success')
# end
# end
#
# # good
# class BooksController < ApplicationController
# def create
# # ...
# redirect_to books_url, notice: t('.success')
# end
# end
#
class I18nLazyLookup < Base
include VisibilityHelp
extend AutoCorrector

MSG = 'Use "lazy" lookup for the text used in controllers.'

def_node_matcher :translate_call?, <<~PATTERN
(send nil? {:translate :t} ${sym_type? str_type?} ...)
PATTERN

def on_send(node)
translate_call?(node) do |key_node|
key = key_node.value
return if key.to_s.start_with?('.')

controller, action = controller_and_action(node)
fatkodima marked this conversation as resolved.
Show resolved Hide resolved
return unless controller && action

scoped_key = get_scoped_key(key_node, controller, action)
return unless key == scoped_key

add_offense(key_node) do |corrector|
unscoped_key = key_node.value.to_s.split('.').last
corrector.replace(key_node, "'.#{unscoped_key}'")
end
end
end

private

def controller_and_action(node)
action_node = node.each_ancestor(:def).first
return unless action_node && node_visibility(action_node) == :public

controller_node = node.each_ancestor(:class).first
return unless controller_node && controller_node.identifier.source.end_with?('Controller')

[controller_node, action_node]
end

def get_scoped_key(key_node, controller, action)
path = controller_path(controller).tr('/', '.')
action_name = action.method_name
key = key_node.value.to_s.split('.').last

"#{path}.#{action_name}.#{key}"
end

def controller_path(controller)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this only for controllers? How about mailers, models, serializers etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I remember correctly, "Lazy Lookup" works only inside views and controllers? https://guides.rubyonrails.org/i18n.html#lazy-lookup

Copy link
Member

@pirj pirj Dec 25, 2021

Choose a reason for hiding this comment

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

Fair enough. Sadly, the guide confusingly tells "Rails implements a convenient way to look up the locale inside views". - edit they also tell "can also be used in controllers", so it's fine.

Views are unlikely something we can parse with RuboCop, can we?

So it's all good here 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't seen that rubocop has any view checking code in any cops, so do not know.

module_name = controller.parent_module_name
controller_name = controller.identifier.source

path = if module_name == 'Object'
controller_name
else
"#{module_name}::#{controller_name}"
end

path.delete_suffix('Controller').underscore
end
end
end
end
end
1 change: 1 addition & 0 deletions lib/rubocop/cop/rails_cops.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
require_relative 'rails/helper_instance_variable'
require_relative 'rails/http_positional_arguments'
require_relative 'rails/http_status'
require_relative 'rails/i18n_lazy_lookup'
require_relative 'rails/i18n_locale_assignment'
require_relative 'rails/ignored_skip_action_filter_option'
require_relative 'rails/index_by'
Expand Down
114 changes: 114 additions & 0 deletions spec/rubocop/cop/rails/i18n_lazy_lookup_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Rails::I18nLazyLookup, :config do
it 'registers an offense and corrects when using translation helpers with the key scoped to controller and action' do
expect_offense(<<~RUBY)
class FooController
def action
t 'foo.action.key'
^^^^^^^^^^^^^^^^ Use "lazy" lookup for the text used in controllers.
translate 'foo.action.key'
^^^^^^^^^^^^^^^^ Use "lazy" lookup for the text used in controllers.
end
end
RUBY

expect_correction(<<~RUBY)
class FooController
def action
t '.key'
translate '.key'
end
end
RUBY
end

it 'does not register an offense when translation methods scoped to `I18n`' do
expect_no_offenses(<<~RUBY)
class FooController
def action
I18n.t 'foo.action.key'
I18n.translate 'foo.action.key'
end
end
RUBY
end

it 'does not register an offense when not inside controller' do
expect_no_offenses(<<~RUBY)
class FooService
def do_something
t 'foo_service.do_something.key'
end
end
RUBY
end

it 'does not register an offense when not inside controller action' do
expect_no_offenses(<<~RUBY)
class FooController
private

def action
t 'foo.action.key'
end
end
RUBY
end

it 'does not register an offense when translating key not scoped to controller and action' do
expect_no_offenses(<<~RUBY)
class FooController
def action
t 'one.two.key'
end
end
RUBY
end

it 'does not register an offense when using "lazy" translation' do
expect_no_offenses(<<~RUBY)
class FooController
def action
t '.key'
end
end
RUBY
end

it 'does not register an offense when translation key is not a string nor a symbol' do
expect_no_offenses(<<~RUBY)
class FooController
def action
t ['foo.action.key']
Copy link
Member

Choose a reason for hiding this comment

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

Is this legal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rails' t helpers delegate to I18n.translate and this is used for bulk translates.

Copy link
Member

Choose a reason for hiding this comment

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

Tested this and found a couple of notable things:

  1. t ['.key'] won't work as doesn't Rails allow lazy lookup for bulk lookup. So this example in question is perfectly fine.

  2. What troubled me is the implementation of translate in controller code:

> ['.a', '.b'].start_with?('.')
NoMethodError: undefined method `start_with?' for [".a", ".b"]:Array

It would work before Rails 7, because it was implemented as:

if key.to_s.first == "."

where key is ['.a', '.b'], but I suspect it would fail in Rails 7.

  1. AbstractController::Translation is not included to ActionController::API, only to ActionController::Base](https://github.com/rails/rails/blob/main/actionpack/lib/action_controller/base.rb). It doesn't make the cop unsafe, since it's not us who suggest using t, it's already there in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for investigation 👍
Yes, I think no one actually uses this bulk feature, since, as you mentioned, it doesn't work.

This test actually tests that no offenses are generated for this case. Only when the keys is a string or a symbol.

So, wdyt, should I remove this test case or left it as is?

Copy link
Member

Choose a reason for hiding this comment

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

It's fine to keep it, just to make sure we don't add to the existing mess 😄

t key
end
end
RUBY
end

it 'handles scoped controllers' do
expect_offense(<<~RUBY)
module Bar
class FooController
def action
t 'bar.foo.action.key'
^^^^^^^^^^^^^^^^^^^^ Use "lazy" lookup for the text used in controllers.
t 'foo.action.key'
end
end
end
RUBY

expect_correction(<<~RUBY)
module Bar
class FooController
def action
t '.key'
t 'foo.action.key'
end
end
end
RUBY
end
end