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 Rails/HashKeyValueRoute cop #1329

Closed
wants to merge 1 commit into from
Closed
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_rails_hash_key_value_route_cop.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#1329](https://github.com/rubocop/rubocop-rails/pull/1329): Add `Rails/HashKeyValueRoute` cop. ([@r7kamura][])
9 changes: 9 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,15 @@ Rails/HasManyOrHasOneDependent:
Include:
- app/models/**/*.rb

Rails/HashKeyValueRoute:
Description: 'Avoid Hash key-value routing.'
Enabled: pending
Safe: false
VersionAdded: '<<next>>'
Include:
- config/routes.rb
- config/routes/**/*.rb

Rails/HelperInstanceVariable:
Description: 'Do not use instance variables in helpers.'
Enabled: true
Expand Down
65 changes: 65 additions & 0 deletions lib/rubocop/cop/rails/hash_key_value_route.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Rails
# Avoid Hash key-value routing.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add the deprecation from 7.2 as a rational for this cop? This is very sparse.

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, I've added a little more explanation based on the Rails' deprecation message.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's good, thanks! And my bad, it'll only be deprecated in 8.0 (removed in 8.1) like you correctly wrote.

# Drawing a route in this style will be removed in Rails 8.1.
#
# @safety
# This cop is unsafe because it is implemented on the assumption that
# the first element of Hash always contains path and endpoint, based on
# the background that it is often so written.
#
# @example
# # bad
# get '/users' => 'users#index'
#
# # good
# get '/users', to: 'users#index'
#
# # bad
# mount MyApp => '/my_app'
#
# # good
# mount MyApp, at: '/my_app'
class HashKeyValueRoute < Base
extend AutoCorrector

MSG = 'Avoid Hash key-value routing.'

RESTRICT_ON_SEND = %i[delete get match mount options patch post put].freeze

# @!method hash_key_value_route(node)
def_node_matcher :hash_key_value_route, <<~PATTERN
(send
nil?
_
(hash
$(pair $_ $_)
...
)
)
PATTERN

def on_send(node)
hash_key_value_route(node) do |pair, key, value|
add_offense(pair) do |corrector|
corrector.replace(pair, "#{key.source}, #{option_name(node)}: #{value.source}")
end
end
end

private

def option_name(node)
if node.method?(:mount)
'at'
else
'to'
end
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 @@ -59,6 +59,7 @@
require_relative 'rails/freeze_time'
require_relative 'rails/has_and_belongs_to_many'
require_relative 'rails/has_many_or_has_one_dependent'
require_relative 'rails/hash_key_value_route'
require_relative 'rails/helper_instance_variable'
require_relative 'rails/http_positional_arguments'
require_relative 'rails/http_status'
Expand Down
50 changes: 50 additions & 0 deletions spec/rubocop/cop/rails/hash_key_value_route_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Rails::HashKeyValueRoute, :config do
context 'when using hash key-value route' do
it 'registers an offense' do
expect_offense(<<~RUBY)
get '/foos' => 'foos#index'
^^^^^^^^^^^^^^^^^^^^^^^ Avoid Hash key-value routing.
RUBY

expect_correction(<<~RUBY)
get '/foos', to: 'foos#index'
RUBY
end
end

context 'with other options' do
it 'registers an offense' do
expect_offense(<<~RUBY)
get '/foos' => 'foos#index', as: 'foos'
^^^^^^^^^^^^^^^^^^^^^^^ Avoid Hash key-value routing.
RUBY

expect_correction(<<~RUBY)
get '/foos', to: 'foos#index', as: 'foos'
RUBY
end
end

context 'with `mount`' do
it 'registers an offense' do
expect_offense(<<~RUBY)
mount MyApp => '/my_app'
^^^^^^^^^^^^^^^^^^ Avoid Hash key-value routing.
RUBY

expect_correction(<<~RUBY)
mount MyApp, at: '/my_app'
RUBY
end
end

context 'without hash key-value route' do
it 'does not register an offense' do
expect_no_offenses(<<~RUBY)
get '/foos', to: 'foos#index'
RUBY
end
end
end