From 5e3303d1e7f02d8eba9350566646326ad0819c18 Mon Sep 17 00:00:00 2001 From: Leo Arnold Date: Sun, 21 Nov 2021 02:52:28 +0100 Subject: [PATCH] Add new `Rails/RootPathnameMethods` cop `Rails.root` is an instance of `Pathname`. So instead of ```ruby File.open(Rails.root.join('db', 'schema.rb')) File.open(Rails.root.join('db', 'schema.rb'), 'w') File.read(Rails.root.join('db', 'schema.rb')) File.binread(Rails.root.join('db', 'schema.rb')) File.write(Rails.root.join('db', 'schema.rb'), content) File.binwrite(Rails.root.join('db', 'schema.rb'), content) ``` we can simply write ```ruby Rails.root.join('db', 'schema.rb').open Rails.root.join('db', 'schema.rb').open('w') Rails.root.join('db', 'schema.rb').read Rails.root.join('db', 'schema.rb').binread Rails.root.join('db', 'schema.rb').write(content) Rails.root.join('db', 'schema.rb').binwrite(content) ``` This cop works best when used together with [`Style/FileRead`](https://github.com/rubocop/rubocop/pull/10261), [`Style/FileWrite`](https://github.com/rubocop/rubocop/pull/10260), and [`Rails/RootJoinChain`](https://github.com/rubocop/rubocop-rails/pull/586). --- ...ew_add_new_railsrootpathnamemethods_cop.md | 1 + codespell.txt | 1 + config/default.yml | 5 + lib/rubocop-rails.rb | 1 + lib/rubocop/cop/rails/base.rb | 19 ++ lib/rubocop/cop/rails/root_join_chain.rb | 5 - .../cop/rails/root_pathname_methods.rb | 189 ++++++++++++++++++ lib/rubocop/cop/rails_cops.rb | 2 + lib/rubocop/rails/language.rb | 20 ++ .../cop/rails/root_pathname_methods_spec.rb | 92 +++++++++ 10 files changed, 330 insertions(+), 5 deletions(-) create mode 100644 changelog/new_add_new_railsrootpathnamemethods_cop.md create mode 100644 lib/rubocop/cop/rails/base.rb create mode 100644 lib/rubocop/cop/rails/root_pathname_methods.rb create mode 100644 lib/rubocop/rails/language.rb create mode 100644 spec/rubocop/cop/rails/root_pathname_methods_spec.rb diff --git a/changelog/new_add_new_railsrootpathnamemethods_cop.md b/changelog/new_add_new_railsrootpathnamemethods_cop.md new file mode 100644 index 0000000000..38d629c9af --- /dev/null +++ b/changelog/new_add_new_railsrootpathnamemethods_cop.md @@ -0,0 +1 @@ +* [#587](https://github.com/rubocop/rubocop-rails/pull/587): Add new `Rails/RootPathnameMethods` cop. ([@leoarnold][]) diff --git a/codespell.txt b/codespell.txt index 732d1968be..3f92fc0c48 100644 --- a/codespell.txt +++ b/codespell.txt @@ -1 +1,2 @@ developpment +filetest \ No newline at end of file diff --git a/config/default.yml b/config/default.yml index 7c886526eb..0965a3c778 100644 --- a/config/default.yml +++ b/config/default.yml @@ -788,6 +788,11 @@ Rails/RootJoinChain: Enabled: pending VersionAdded: '2.13' +Rails/RootPathnameMethods: + Description: 'Use `Rails.root` IO methods instead of passing it to `File`.' + Enabled: pending + VersionAdded: '<>' + Rails/RootPublicPath: Description: "Favor `Rails.public_path` over `Rails.root` with `'public'`." Enabled: pending diff --git a/lib/rubocop-rails.rb b/lib/rubocop-rails.rb index a5ac04629e..4efaeada93 100644 --- a/lib/rubocop-rails.rb +++ b/lib/rubocop-rails.rb @@ -12,6 +12,7 @@ RuboCop::Rails::Inject.defaults! +require_relative 'rubocop/rails/language' require_relative 'rubocop/cop/rails_cops' RuboCop::Cop::Style::RedundantSelf.singleton_class.prepend( diff --git a/lib/rubocop/cop/rails/base.rb b/lib/rubocop/cop/rails/base.rb new file mode 100644 index 0000000000..6096beccc2 --- /dev/null +++ b/lib/rubocop/cop/rails/base.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # @abstract parent class to Rails cops + class Base < ::RuboCop::Cop::Base + include RuboCop::Rails::Language + + exclude_from_registry + + # Invoke the original inherited hook so our cops are recognized + def self.inherited(subclass) # rubocop:disable Lint/MissingSuper + RuboCop::Cop::Base.inherited(subclass) + end + end + end + end +end diff --git a/lib/rubocop/cop/rails/root_join_chain.rb b/lib/rubocop/cop/rails/root_join_chain.rb index fb28d0c2d3..59f43d394f 100644 --- a/lib/rubocop/cop/rails/root_join_chain.rb +++ b/lib/rubocop/cop/rails/root_join_chain.rb @@ -26,11 +26,6 @@ class RootJoinChain < Base RESTRICT_ON_SEND = %i[join].to_set.freeze - # @!method rails_root?(node) - def_node_matcher :rails_root?, <<~PATTERN - (send (const {nil? cbase} :Rails) {:root :public_path}) - PATTERN - # @!method join?(node) def_node_matcher :join?, <<~PATTERN (send _ :join $...) diff --git a/lib/rubocop/cop/rails/root_pathname_methods.rb b/lib/rubocop/cop/rails/root_pathname_methods.rb new file mode 100644 index 0000000000..25ca23d5c5 --- /dev/null +++ b/lib/rubocop/cop/rails/root_pathname_methods.rb @@ -0,0 +1,189 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # Use `Rails.root` IO methods instead of passing it to `File`. + # + # `Rails.root` is an instance of `Pathname` + # so we can apply many IO methods directly. + # + # This cop works best when used together with + # `Style/FileRead`, `Style/FileWrite` and `Rails/RootJoinChain`. + # + # @example + # # bad + # File.open(Rails.root.join('db', 'schema.rb')) + # File.open(Rails.root.join('db', 'schema.rb'), 'w') + # File.read(Rails.root.join('db', 'schema.rb')) + # File.binread(Rails.root.join('db', 'schema.rb')) + # File.write(Rails.root.join('db', 'schema.rb'), content) + # File.binwrite(Rails.root.join('db', 'schema.rb'), content) + # + # # good + # Rails.root.join('db', 'schema.rb').open + # Rails.root.join('db', 'schema.rb').open('w') + # Rails.root.join('db', 'schema.rb').read + # Rails.root.join('db', 'schema.rb').binread + # Rails.root.join('db', 'schema.rb').write(content) + # Rails.root.join('db', 'schema.rb').binwrite(content) + # + class RootPathnameMethods < Base + extend AutoCorrector + + MSG = '`%s` is a `Pathname` so you can just append `#%s`.' + + DIR_METHODS = %i[ + children + delete + each_child + empty? + entries + exist? + glob + mkdir + open + rmdir + unlink + ].to_set.freeze + + FILE_METHODS = %i[ + atime + basename + binread + binwrite + birthtime + blockdev? + chardev? + chmod + chown + ctime + delete + directory? + dirname + empty? + executable? + executable_real? + exist? + expand_path + extname + file? + fnmatch + fnmatch? + ftype + grpowned? + join + lchmod + lchown + lstat + mtime + open + owned? + pipe? + read + readable? + readable_real? + readlines + readlink + realdirpath + realpath + rename + setgid? + setuid? + size + size? + socket? + split + stat + sticky? + symlink? + sysopen + truncate + unlink + utime + world_readable? + world_writable? + writable? + writable_real? + write + zero? + ].to_set.freeze + + FILE_TEST_METHODS = %i[ + blockdev? + chardev? + directory? + empty? + executable? + executable_real? + exist? + file? + grpowned? + owned? + pipe? + readable? + readable_real? + setgid? + setuid? + size + size? + socket? + sticky? + symlink? + world_readable? + world_writable? + writable? + writable_real? + zero? + ].to_set.freeze + + FILE_UTILS_METHODS = %i[ + chmod + chown + mkdir + mkpath + rmdir + rmtree + ].to_set.freeze + + RESTRICT_ON_SEND = (DIR_METHODS + FILE_METHODS + FILE_TEST_METHODS + FILE_UTILS_METHODS).to_set.freeze + + def_node_matcher :pathname_method, <<~PATTERN + { + (send (const {nil? cbase} :Dir) $DIR_METHODS $_ $...) + (send (const {nil? cbase} {:IO :File}) $FILE_METHODS $_ $...) + (send (const {nil? cbase} :FileTest) $FILE_TEST_METHODS $_ $...) + (send (const {nil? cbase} :FileUtils) $FILE_UTILS_METHODS $_ $...) + } + PATTERN + + def_node_matcher :rails_root_pathname?, <<~PATTERN + { + $#rails_root? + (send $#rails_root? :join ...) + } + PATTERN + + def on_send(node) + evidence(node) do |method, path, args, rails_root| + add_offense(node, message: format(MSG, method: method, rails_root: rails_root.source)) do |corrector| + replacement = "#{path.source}.#{method}" + replacement += "(#{args.map(&:source).join(', ')})" unless args.empty? + + corrector.replace(node, replacement) + end + end + end + + private + + def evidence(node) + return if node.method?(:open) && node.parent&.send_type? + return unless (method, path, args = pathname_method(node)) && (rails_root = rails_root_pathname?(path)) + + yield(method, path, args, rails_root) + end + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index cf7c6c5769..a345d242c2 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -7,6 +7,7 @@ require_relative 'mixin/index_method' require_relative 'mixin/migrations_helper' require_relative 'mixin/target_rails_version' +require_relative 'rails/base' require_relative 'rails/action_controller_test_case' require_relative 'rails/action_filter' @@ -97,6 +98,7 @@ require_relative 'rails/reversible_migration' require_relative 'rails/reversible_migration_method_definition' require_relative 'rails/root_join_chain' +require_relative 'rails/root_pathname_methods' require_relative 'rails/root_public_path' require_relative 'rails/safe_navigation' require_relative 'rails/safe_navigation_with_blank' diff --git a/lib/rubocop/rails/language.rb b/lib/rubocop/rails/language.rb new file mode 100644 index 0000000000..b9cb066f33 --- /dev/null +++ b/lib/rubocop/rails/language.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +module RuboCop + module Rails + # Contains node matchers for common Rails DSL. + module Language + extend RuboCop::NodePattern::Macros + + # @!method rails?(node) + def_node_matcher :rails?, <<~PATTERN + (const {nil? cbase} :Rails) + PATTERN + + # @!method rails_root?(node) + def_node_matcher :rails_root?, <<~PATTERN + (send #rails? {:root :public_path}) + PATTERN + end + end +end diff --git a/spec/rubocop/cop/rails/root_pathname_methods_spec.rb b/spec/rubocop/cop/rails/root_pathname_methods_spec.rb new file mode 100644 index 0000000000..c0bfce36cb --- /dev/null +++ b/spec/rubocop/cop/rails/root_pathname_methods_spec.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::RootPathnameMethods, :config do + { + Dir: described_class::DIR_METHODS, + File: described_class::FILE_METHODS, + FileTest: described_class::FILE_TEST_METHODS, + FileUtils: described_class::FILE_UTILS_METHODS, + IO: described_class::FILE_METHODS + }.each do |receiver, methods| + methods.each do |method| + it "registers an offense when using `#{receiver}.#{method}(Rails.public_path)` (if arity exists)" do + expect_offense(<<~RUBY) + #{receiver}.#{method}(Rails.public_path) + #{'^' * receiver.size}^#{'^' * method.size}^^^^^^^^^^^^^^^^^^^ `Rails.public_path` is a `Pathname` so you can just append `##{method}`. + RUBY + + expect_correction(<<~RUBY) + Rails.public_path.#{method} + RUBY + end + + it "registers an offense when using `::#{receiver}.#{method}(::Rails.root.join(...))` (if arity exists)" do + expect_offense(<<~RUBY) + ::#{receiver}.#{method}(::Rails.root.join('db', 'schema.rb')) + ^^#{'^' * receiver.size}^#{'^' * method.size}^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `::Rails.root` is a `Pathname` so you can just append `##{method}`. + RUBY + + expect_correction(<<~RUBY) + ::Rails.root.join('db', 'schema.rb').#{method} + RUBY + end + + it "registers an offense when using `::#{receiver}.#{method}(::Rails.root.join(...), ...)` (if arity exists)" do + expect_offense(<<~RUBY) + ::#{receiver}.#{method}(::Rails.root.join('db', 'schema.rb'), 20, 5) + ^^#{'^' * receiver.size}^#{'^' * method.size}^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `::Rails.root` is a `Pathname` so you can just append `##{method}`. + RUBY + + expect_correction(<<~RUBY) + ::Rails.root.join('db', 'schema.rb').#{method}(20, 5) + RUBY + end + end + end + + # This is handled by `Rails/RootJoinChain` + it 'does not register an offense when using `File.read(Rails.root.join(...).join(...))`' do + expect_no_offenses(<<~RUBY) + File.read(Rails.root.join('db').join('schema.rb')) + RUBY + end + + # This is handled by `Style/FileRead` + it 'does not register an offense when using `File.open(Rails.root.join(...)).read`' do + expect_no_offenses(<<~RUBY) + File.open(Rails.root.join('db', 'schema.rb')).read + RUBY + end + + # This is handled by `Style/FileRead` + it 'does not register an offense when using `File.open(Rails.root.join(...)).binread`' do + expect_no_offenses(<<~RUBY) + File.open(Rails.root.join('db', 'schema.rb')).binread + RUBY + end + + # This is handled by `Style/FileWrite` + it 'does not register an offense when using `File.open(Rails.root.join(...)).write(content)`' do + expect_no_offenses(<<~RUBY) + File.open(Rails.root.join('db', 'schema.rb')).write(content) + RUBY + end + + # This is handled by `Style/FileWrite` + it 'does not register an offense when using `File.open(Rails.root.join(...)).binwrite(content)`' do + expect_no_offenses(<<~RUBY) + File.open(Rails.root.join('db', 'schema.rb')).binwrite(content) + RUBY + end + + it 'registers an offense when using `File.open(Rails.root.join(...), ...)` inside an iterator' do + expect_offense(<<~RUBY) + files.map { |file| File.open(Rails.root.join('db', file), 'wb') } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `Rails.root` is a `Pathname` so you can just append `#open`. + RUBY + + expect_correction(<<~RUBY) + files.map { |file| Rails.root.join('db', file).open('wb') } + RUBY + end +end