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/MultipleRoutePaths cop #1407

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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_rails_multiple_route_paths_cop.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#1407](https://github.com/rubocop/rubocop-rails/pull/1407): Add new `Rails/MultipleRoutePaths` cop. ([@koic][])
6 changes: 6 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,12 @@ Rails/MigrationClassName:
Include:
- db/**/*.rb

Rails/MultipleRoutePaths:
Description: 'Checks for mapping a route with multiple paths, which is deprecated and will be removed in Rails 8.1.'
Enabled: pending
Severity: warning
VersionAdded: '<<next>>'

Rails/NegateInclude:
Description: 'Prefer `collection.exclude?(obj)` over `!collection.include?(obj)`.'
StyleGuide: 'https://rails.rubystyle.guide#exclude'
Expand Down
20 changes: 20 additions & 0 deletions lib/rubocop/cop/mixin/routes_helper.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# frozen_string_literal: true

module RuboCop
module Cop
# Common functionality for cops working with routes.
module RoutesHelper
extend NodePattern::Macros

HTTP_METHODS = %i[get post put patch delete].freeze

def_node_matcher :routes_draw?, <<~PATTERN
(send (send _ :routes) :draw)
PATTERN

def within_routes?(node)
node.each_ancestor(:block).any? { |block| routes_draw?(block.send_node) }
end
end
end
end
10 changes: 1 addition & 9 deletions lib/rubocop/cop/rails/match_route.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@ module Rails
# match 'photos/:id', to: 'photos#show', via: :all
#
class MatchRoute < Base
include RoutesHelper
extend AutoCorrector

MSG = 'Use `%<http_method>s` instead of `match` to define a route.'
RESTRICT_ON_SEND = %i[match].freeze
HTTP_METHODS = %i[get post put patch delete].freeze

def_node_matcher :match_method_call?, <<~PATTERN
(send nil? :match $_ $(hash ...) ?)
Expand Down Expand Up @@ -60,14 +60,6 @@ def register_offense(node, http_method)
end
end

def_node_matcher :routes_draw?, <<~PATTERN
(send (send _ :routes) :draw)
PATTERN

def within_routes?(node)
node.each_ancestor(:block).any? { |a| routes_draw?(a.send_node) }
end

def extract_via(node)
via_pair = via_pair(node)
return %i[get] unless via_pair
Expand Down
50 changes: 50 additions & 0 deletions lib/rubocop/cop/rails/multiple_route_paths.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Rails
# Checks for mapping a route with multiple paths, which is deprecated and will be removed in Rails 8.1.
#
# @example
#
# # bad
# get '/users', '/other_path', to: 'users#index'
#
# # good
# get '/users', to: 'users#index'
# get '/other_path', to: 'users#index'
#
class MultipleRoutePaths < Base
include RoutesHelper
extend AutoCorrector

MSG = 'Use separate routes instead of combining multiple route paths in a single route.'
RESTRICT_ON_SEND = HTTP_METHODS

IGNORED_ARGUMENT_TYPES = %i[array hash].freeze

def on_send(node)
return unless within_routes?(node)

route_paths = node.arguments.reject { |argument| IGNORED_ARGUMENT_TYPES.include?(argument.type) }
return if route_paths.count < 2

add_offense(node) do |corrector|
corrector.replace(node, migrate_to_multiple_routes(node, route_paths))
end
end

private

def migrate_to_multiple_routes(node, route_paths)
rest = route_paths.last.source_range.end.join(node.source_range.end).source
indentation = ' ' * node.source_range.column

route_paths.map do |route_path|
"#{node.method_name} #{route_path.source}#{rest}"
end.join("\n#{indentation}")
end
end
end
end
end
2 changes: 2 additions & 0 deletions lib/rubocop/cop/rails_cops.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
require_relative 'mixin/enforce_superclass'
require_relative 'mixin/index_method'
require_relative 'mixin/migrations_helper'
require_relative 'mixin/routes_helper'
require_relative 'mixin/target_rails_version'

require_relative 'rails/action_controller_flash_before_render'
Expand Down Expand Up @@ -77,6 +78,7 @@
require_relative 'rails/mailer_name'
require_relative 'rails/match_route'
require_relative 'rails/migration_class_name'
require_relative 'rails/multiple_route_paths'
require_relative 'rails/negate_include'
require_relative 'rails/not_null_column'
require_relative 'rails/order_by_id'
Expand Down
112 changes: 112 additions & 0 deletions spec/rubocop/cop/rails/multiple_route_paths_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Rails::MultipleRoutePaths, :config do
it 'registers an offense when using a route with multiple string route paths' do
expect_offense(<<~RUBY)
Rails.application.routes.draw do
get '/users', '/other_path/users', '/another_path/users'
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use separate routes instead of combining multiple route paths in a single route.
end
RUBY

expect_correction(<<~RUBY)
Rails.application.routes.draw do
get '/users'
get '/other_path/users'
get '/another_path/users'
end
RUBY
end

it 'registers an offense when using a route with multiple route paths with option' do
expect_offense(<<~RUBY)
Rails.application.routes.draw do
get '/users', '/other_path/users', '/another_path/users', to: 'users#index'
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use separate routes instead of combining multiple route paths in a single route.
end
RUBY

expect_correction(<<~RUBY)
Rails.application.routes.draw do
get '/users', to: 'users#index'
get '/other_path/users', to: 'users#index'
get '/another_path/users', to: 'users#index'
end
RUBY
end

it 'registers an offense when using a route with multiple route paths with splat option' do
expect_offense(<<~RUBY)
Rails.application.routes.draw do
get '/users', '/other_path/users', '/another_path/users', **options
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use separate routes instead of combining multiple route paths in a single route.
end
RUBY

expect_correction(<<~RUBY)
Rails.application.routes.draw do
get '/users', **options
get '/other_path/users', **options
get '/another_path/users', **options
end
RUBY
end

it 'registers an offense when using a route with multiple symbol route paths' do
expect_offense(<<~RUBY)
Rails.application.routes.draw do
get :resend, :generate_new_password
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use separate routes instead of combining multiple route paths in a single route.
end
RUBY

expect_correction(<<~RUBY)
Rails.application.routes.draw do
get :resend
get :generate_new_password
end
RUBY
end

it 'does not register an offense when using single string path method calls' do
expect_no_offenses(<<~RUBY)
Rails.application.routes.draw do
get '/users'
get '/other_path/users'
get '/another_path/users'
end
RUBY
end

it 'does not register an offense when using single string path with option method calls' do
expect_no_offenses(<<~RUBY)
Rails.application.routes.draw do
get '/users', to: 'users#index'
get '/other_path/users', to: 'users#index'
get '/another_path/users', to: 'users#index'
end
RUBY
end

it 'does not register an offense when using single string path with array literal' do
expect_no_offenses(<<~RUBY)
Rails.application.routes.draw do
get '/other_path/users', []
end
RUBY
end

it 'does not register an offense when using single route path with no arguments' do
expect_no_offenses(<<~RUBY)
Rails.application.routes.draw do
get
end
RUBY
end

it 'does not register an offense when not within routes' do
expect_no_offenses(<<~RUBY)
get '/users', '/other_path/users', '/another_path/users'
RUBY
end
end
Loading