From 95c2fe4fdad3669683965a69046dd1e0077a9dc1 Mon Sep 17 00:00:00 2001 From: Leo Arnold Date: Sun, 21 Nov 2021 04:59:43 +0100 Subject: [PATCH] Add new `Rails/RootJoinChain` cop 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') ``` --- CHANGELOG.md | 1 + .../new_add_new_railsrootjoinchain_cop.md | 1 + config/default.yml | 5 ++ lib/rubocop/cop/rails/root_join_chain.rb | 70 ++++++++++++++++ lib/rubocop/cop/rails_cops.rb | 3 +- .../rubocop/cop/rails/root_join_chain_spec.rb | 81 +++++++++++++++++++ 6 files changed, 160 insertions(+), 1 deletion(-) create mode 100644 changelog/new_add_new_railsrootjoinchain_cop.md create mode 100644 lib/rubocop/cop/rails/root_join_chain.rb create mode 100644 spec/rubocop/cop/rails/root_join_chain_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 4344effbf7..0947927d1b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -484,3 +484,4 @@ [@theunraveler]: https://github.com/theunraveler [@pirj]: https://github.com/pirj [@vitormd]: https://github.com/vitormd +[@leoarnold]: https://github.com/leoarnold diff --git a/changelog/new_add_new_railsrootjoinchain_cop.md b/changelog/new_add_new_railsrootjoinchain_cop.md new file mode 100644 index 0000000000..75db5f665c --- /dev/null +++ b/changelog/new_add_new_railsrootjoinchain_cop.md @@ -0,0 +1 @@ +* [#586](https://github.com/rubocop/rubocop-rails/pull/586): Add new `Rails/RootJoinChain` cop. ([@leoarnold][]) diff --git a/config/default.yml b/config/default.yml index 6c96baa276..d916e83268 100644 --- a/config/default.yml +++ b/config/default.yml @@ -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: '<>' + Rails/SafeNavigation: Description: "Use Ruby's safe navigation operator (`&.`) instead of `try!`." Enabled: true diff --git a/lib/rubocop/cop/rails/root_join_chain.rb b/lib/rubocop/cop/rails/root_join_chain.rb new file mode 100644 index 0000000000..1c2c612231 --- /dev/null +++ b/lib/rubocop/cop/rails/root_join_chain.rb @@ -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 `%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 diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index 42d154a6f2..a642abd347 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -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' @@ -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' diff --git a/spec/rubocop/cop/rails/root_join_chain_spec.rb b/spec/rubocop/cop/rails/root_join_chain_spec.rb new file mode 100644 index 0000000000..08a5441b28 --- /dev/null +++ b/spec/rubocop/cop/rails/root_join_chain_spec.rb @@ -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