Skip to content

Commit

Permalink
Add new Rails/RootJoinChain cop
Browse files Browse the repository at this point in the history
Instead of chaining `#join` on `Rails.root` or `Rails.public_path`

```ruby
Rails.root.join('db').join('schema.rb')
Rails.root.join('db').join(migrate).join('migration.rb')
Rails.public_path.join('path').join('file.pdf')
Rails.public_path.join('path').join(to).join('file.pdf')
```

we can combine all arguments into a single join

```ruby
Rails.root.join('db', 'schema.rb')
Rails.root.join('db', migrate, 'migration.rb')
Rails.public_path.join('path', 'file.pdf')
Rails.public_path.join('path', to, 'file.pdf')
```
  • Loading branch information
leoarnold committed Nov 23, 2021
1 parent 19369de commit 95c2fe4
Show file tree
Hide file tree
Showing 6 changed files with 160 additions and 1 deletion.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -484,3 +484,4 @@
[@theunraveler]: https://github.com/theunraveler
[@pirj]: https://github.com/pirj
[@vitormd]: https://github.com/vitormd
[@leoarnold]: https://github.com/leoarnold
1 change: 1 addition & 0 deletions changelog/new_add_new_railsrootjoinchain_cop.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#586](https://github.com/rubocop/rubocop-rails/pull/586): Add new `Rails/RootJoinChain` cop. ([@leoarnold][])
5 changes: 5 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -701,6 +701,11 @@ Rails/ReversibleMigrationMethodDefinition:
Include:
- db/migrate/*.rb

Rails/RootJoinChain:
Description: 'Use a single `#join` instead of chaining on `Rails.root` or `Rails.public_path`.'
Enabled: pending
VersionAdded: '<<next>>'

Rails/SafeNavigation:
Description: "Use Ruby's safe navigation operator (`&.`) instead of `try!`."
Enabled: true
Expand Down
70 changes: 70 additions & 0 deletions lib/rubocop/cop/rails/root_join_chain.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Rails
# Use a single `#join` instead of chaining on `Rails.root` or `Rails.public_path`.
#
# @example
# # bad
# Rails.root.join('db').join('schema.rb')
# Rails.root.join('db').join(migrate).join('migration.rb')
# Rails.public_path.join('path').join('file.pdf')
# Rails.public_path.join('path').join(to).join('file.pdf')
#
# # good
# Rails.root.join('db', 'schema.rb')
# Rails.root.join('db', migrate, 'migration.rb')
# Rails.public_path.join('path', 'file.pdf')
# Rails.public_path.join('path', to, 'file.pdf')
#
class RootJoinChain < Base
extend AutoCorrector
include RangeHelp

MSG = 'Use `%<root>s.join(...)` instead of chaining `#join` calls.'

RESTRICT_ON_SEND = %i[join].to_set.freeze

def_node_matcher :rails_root?, <<~PATTERN
(send (const {nil? cbase} :Rails) {:root :public_path})
PATTERN

def_node_matcher :join?, <<~PATTERN
(send _ :join $...)
PATTERN

def on_send(node)
evidence(node) do |rails_node, args|
add_offense(node, message: format(MSG, root: rails_node.source)) do |corrector|
range = range_between(rails_node.loc.selector.end_pos, node.loc.expression.end_pos)
replacement = ".join(#{args.map(&:source).join(', ')})"

corrector.replace(range, replacement)
end
end
end

private

def evidence(node)
# Are we at the *end* of the join chain?
return if join?(node.parent)
# Is there only one join?
return if rails_root?(node.receiver)

all_args = []

while (args = join?(node))
all_args = args + all_args
node = node.receiver
end

rails_root?(node) do
yield(node, all_args)
end
end
end
end
end
end
3 changes: 2 additions & 1 deletion lib/rubocop/cop/rails_cops.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
require_relative 'rails/active_record_callbacks_order'
require_relative 'rails/active_record_override'
require_relative 'rails/active_support_aliases'
require_relative 'rails/schema_comment'
require_relative 'rails/add_column_index'
require_relative 'rails/after_commit_override'
require_relative 'rails/application_controller'
Expand Down Expand Up @@ -85,9 +84,11 @@
require_relative 'rails/require_dependency'
require_relative 'rails/reversible_migration'
require_relative 'rails/reversible_migration_method_definition'
require_relative 'rails/root_join_chain'
require_relative 'rails/safe_navigation'
require_relative 'rails/safe_navigation_with_blank'
require_relative 'rails/save_bang'
require_relative 'rails/schema_comment'
require_relative 'rails/scope_args'
require_relative 'rails/short_i18n'
require_relative 'rails/skips_model_validations'
Expand Down
81 changes: 81 additions & 0 deletions spec/rubocop/cop/rails/root_join_chain_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Rails::RootJoinChain, :config do
it 'does not register an offense for `Rails.root.join(...)`' do
expect_no_offenses(<<~RUBY)
Rails.root.join('db', 'schema.rb')
RUBY
end

it 'registers and offense and corrects for `::Rails.root.join(...).join(...)`' do
expect_offense(<<~RUBY)
::Rails.root.join('db').join('sch' + 'ema.rb')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `::Rails.root.join(...)` instead of chaining `#join` calls.
RUBY

expect_correction(<<~RUBY)
::Rails.root.join('db', 'sch' + 'ema.rb')
RUBY
end

it 'registers and offense and corrects for `::Rails.root.join(...).join(...).read`' do
expect_offense(<<~RUBY)
::Rails.root.join('db').join('sch' + 'ema.rb').read
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `::Rails.root.join(...)` instead of chaining `#join` calls.
RUBY

expect_correction(<<~RUBY)
::Rails.root.join('db', 'sch' + 'ema.rb').read
RUBY
end

it 'registers and offense and corrects for `Rails.root.join(...).join(...)`' do
expect_offense(<<~RUBY)
Rails.root.join('db').join('sch' + 'ema.rb')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `Rails.root.join(...)` instead of chaining `#join` calls.
RUBY

expect_correction(<<~RUBY)
Rails.root.join('db', 'sch' + 'ema.rb')
RUBY
end

it 'registers and offense and corrects for `Rails.root` with any number of joins greater one' do
expect_offense(<<~RUBY)
Rails.root.join.join.join('db').join(migrate).join.join("migration.\#{rb}")
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `Rails.root.join(...)` instead of chaining `#join` calls.
RUBY

expect_correction(<<~RUBY)
Rails.root.join('db', migrate, "migration.\#{rb}")
RUBY
end

it 'does not register an offense for `Rails.public_path.join(...)`' do
expect_no_offenses(<<~RUBY)
Rails.public_path.join('path', 'file.pdf')
RUBY
end

it 'registers and offense and corrects for `Rails.public_path.join(...).join(...)`' do
expect_offense(<<~RUBY)
Rails.public_path.join('path').join('fi' + 'le.pdf')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `Rails.public_path.join(...)` instead of chaining `#join` calls.
RUBY

expect_correction(<<~RUBY)
Rails.public_path.join('path', 'fi' + 'le.pdf')
RUBY
end

it 'registers and offense and corrects for `Rails.public_path` with any number of joins greater one' do
expect_offense(<<~RUBY)
Rails.public_path.join.join.join('path').join(to).join.join("file.\#{pdf}")
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `Rails.public_path.join(...)` instead of chaining `#join` calls.
RUBY

expect_correction(<<~RUBY)
Rails.public_path.join('path', to, "file.\#{pdf}")
RUBY
end
end

0 comments on commit 95c2fe4

Please sign in to comment.