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.

Sign up for GitHub

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

[WIP] Updating require_hardcoding_lib AST to report an offense when a relative path is used to lib/ (and, not specifically focus on require calls) #38

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[WIP] Updating require_hardcoding_lib AST to report an offense when a relative path is used to lib/ (and, not specifically focus on require calls) #38

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 8 additions & 9 deletions lib/rubocop/cop/packaging/require_hardcoding_lib.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,14 @@ class RequireHardcodingLib < Base

# This is the message that will be displayed when RuboCop::Packaging
# finds an offense of using `require` with relative path to lib.
MSG = "Avoid using `require` with relative path to `lib/`. " \
"Use `require` with absolute path instead."
MSG = "Avoid using relative path to `lib/`. " \
"Use absolute path instead."

def_node_matcher :require?, <<~PATTERN
{(send nil? :require (str #falls_in_lib?))
(send nil? :require (send (const nil? :File) :expand_path (str #falls_in_lib?) (send nil? :__dir__)))
(send nil? :require (send (const nil? :File) :expand_path (str #falls_in_lib_using_file?) (str _)))
(send nil? :require (send (send (const nil? :File) :dirname {(str _) (send nil? _)}) :+ (str #falls_in_lib_with_file_dirname_plus_str?)))
(send nil? :require (dstr (begin (send (const nil? :File) :dirname {(str _) (send nil? _)})) (str #falls_in_lib_with_file_dirname_plus_str?)))}
def_node_matcher :File?, <<~PATTERN
{(send (const nil? :File) :expand_path (str #falls_in_lib?) (send nil? :__dir__))
(send (const nil? :File) :expand_path (str #falls_in_lib_using_file?) (str _))
(send (send (const nil? :File) :dirname {(str _) (send nil? _)}) :+ (str #falls_in_lib_with_file_dirname_plus_str?))
(dstr (begin (send (const nil? :File) :dirname {(str _) (send nil? _)})) (str #falls_in_lib_with_file_dirname_plus_str?))}
PATTERN

# Extended from the Base class.
Expand All @@ -65,7 +64,7 @@ def on_new_investigation
# More about the `#on_send` method can be found here:
# https://github.com/rubocop-hq/rubocop-ast/blob/08d0f49a47af1e9a30a6d8f67533ba793c843d67/lib/rubocop/ast/traversal.rb#L112
def on_send(node)
return unless require?(node)
return unless File?(node)
Comment on lines -68 to +67
Copy link
Owner

Choose a reason for hiding this comment

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

We need a different name here, File is not very clear as to what it implies or means. Also preferably something that's not capitalized. Hope that makes sense?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, of course, thanks for PTO. :)


add_offense(node) do |corrector|
corrector.replace(node, good_require_call)
Expand Down
155 changes: 32 additions & 123 deletions spec/rubocop/cop/packaging/require_hardcoding_lib_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,45 +5,9 @@

let(:project_root) { RuboCop::ConfigLoader.project_root }

context "when `require` call lies outside spec/" do
let(:filename) { "#{project_root}/spec/foo_spec.rb" }
let(:source) { "require '../lib/foo.rb'" }

it "registers an offense" do
expect_offense(<<~RUBY, filename)
#{source}
#{"^" * source.length} #{message}
RUBY

expect_correction(<<~RUBY)
require "foo.rb"
RUBY
end
end

context "when `require` call after using `unshift` lies outside spec/" do
let(:filename) { "#{project_root}/tests/foo/bar.rb" }
let(:source) { <<~RUBY.chomp }
$:.unshift('../../lib')
require '../../lib/foo/bar'
RUBY

it "registers an offense" do
expect_offense(<<~RUBY, filename)
#{source}
#{"^" * 27} #{message}
RUBY

expect_correction(<<~RUBY)
$:.unshift('../../lib')
require "foo/bar"
RUBY
end
end

context "when `require` call uses File#expand_path method with __FILE__" do
Comment on lines -8 to -44
Copy link
Owner

Choose a reason for hiding this comment

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

I expect these tests to be still working, really. Might need some fixing but in the end, it should work, too (even as-is, from a quick POV). Because we're essentially just generalizing the entire set of calls and the above tests are really just a subset of those. So I expect them to work + I expect new tests to be added (the generalized one, mentioned in the issue, for example).

context "when use of File#expand_path method with __FILE__" do
let(:filename) { "#{project_root}/spec/foo.rb" }
let(:source) { "require File.expand_path('../../lib/foo', __FILE__)" }
let(:source) { "File.expand_path('../../lib/foo', __FILE__)" }

it "registers an offense" do
expect_offense(<<~RUBY, filename)
Expand All @@ -57,9 +21,9 @@
end
end

context "when `require` call uses File#expand_path method with __dir__" do
context "when use of File#expand_path method with __dir__" do
let(:filename) { "#{project_root}/test/foo/bar/qux_spec.rb" }
let(:source) { "require File.expand_path('../../../lib/foo/bar/baz/qux', __dir__)" }
let(:source) { "File.expand_path('../../../lib/foo/bar/baz/qux', __dir__)" }

it "registers an offense" do
expect_offense(<<~RUBY, filename)
Expand All @@ -73,9 +37,9 @@
end
end

context "when `require` call uses File#dirname method with __FILE__" do
context "when use of File#dirname method with __FILE__" do
let(:filename) { "#{project_root}/specs/baz/qux_spec.rb" }
let(:source) { "require File.dirname(__FILE__) + '/../../lib/baz/qux'" }
let(:source) { "File.dirname(__FILE__) + '/../../lib/baz/qux'" }

it "registers an offense" do
expect_offense(<<~RUBY, filename)
Expand All @@ -89,9 +53,9 @@
end
end

context "when `require` call uses File#dirname method with __FILE__ with interpolation" do
context "when use of File#dirname method with __FILE__ with interpolation" do
let(:filename) { "#{project_root}/specs/baz/qux_spec.rb" }
let(:source) { 'require "#{File.dirname(__FILE__)}/../../lib/baz/qux"' } # rubocop:disable Lint/InterpolationCheck
let(:source) { '"#{File.dirname(__FILE__)}/../../lib/baz/qux"' } # rubocop:disable Lint/InterpolationCheck

it "registers an offense" do
expect_offense(<<~RUBY, filename)
Expand All @@ -105,9 +69,9 @@
end
end

context "when `require` call uses File#dirname method with __dir__" do
context "when use of File#dirname method with __dir__" do
let(:filename) { "#{project_root}/spec/foo.rb" }
let(:source) { "require File.dirname(__dir__) + '/../lib/foo'" }
let(:source) { "File.dirname(__dir__) + '/../lib/foo'" }

it "registers an offense" do
expect_offense(<<~RUBY, filename)
Expand All @@ -121,9 +85,9 @@
end
end

context "when `require` call uses File#dirname method with __dir__ with interpolation" do
context "when use of File#dirname method with __dir__ with interpolation" do
let(:filename) { "#{project_root}/spec/foo.rb" }
let(:source) { 'require "#{File.dirname(__dir__)}/../lib/foo"' } # rubocop:disable Lint/InterpolationCheck
let(:source) { '"#{File.dirname(__dir__)}/../lib/foo"' } # rubocop:disable Lint/InterpolationCheck

it "registers an offense" do
expect_offense(<<~RUBY, filename)
Expand All @@ -137,64 +101,9 @@
end
end

context "when the `require` call doesn't use relative path" do
let(:filename) { "#{project_root}/spec/bar_spec.rb" }
let(:source) { "require 'bar'" }

it "does not register an offense" do
expect_no_offenses(<<~RUBY, filename)
#{source}
RUBY
end
end

context "when the `require` call lies inside test/" do
let(:filename) { "#{project_root}/test/bar/foo_spec.rb" }
let(:source) { "require '../foo'" }

it "does not register an offense" do
expect_no_offenses(<<~RUBY, filename)
#{source}
RUBY
end
end

context "when the `require` call is made to lib/ but it lies under spec/" do
let(:filename) { "#{project_root}/spec/lib/baz/qux_spec.rb" }
let(:source) { "require '../../lib/foo'" }

it "does not register an offense" do
expect_no_offenses(<<~RUBY, filename)
#{source}
RUBY
end
end

context "when the `require` call is made to lib/ from inside lib/ itself" do
let(:filename) { "#{project_root}/lib/foo/bar.rb" }
let(:source) { "require '../foo'" }

it "does not register an offense" do
expect_no_offenses(<<~RUBY, filename)
#{source}
RUBY
end
end

context "when the `require` call is made from the gemspec file" do
let(:filename) { "#{project_root}/foo.gemspec" }
let(:source) { "require 'lib/foo/version'" }

it "does not register an offense" do
expect_no_offenses(<<~RUBY, filename)
#{source}
RUBY
end
end

context "when `require` call uses File#expand_path method with __FILE__ but lies inside lib/" do
context "when use of File#expand_path method with __FILE__ but lies inside lib/" do
Comment on lines -140 to +104
Copy link
Owner

Choose a reason for hiding this comment

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

Same here.

let(:filename) { "#{project_root}/lib/foo/bar.rb" }
let(:source) { "require File.expand_path('../../foo', __FILE__)" }
let(:source) { "File.expand_path('../../foo', __FILE__)" }

it "does not register an offense" do
expect_no_offenses(<<~RUBY, filename)
Expand All @@ -203,9 +112,9 @@
end
end

context "when `require` call uses File#expand_path method with __FILE__ and is made from the gemspec file" do
context "when use of File#expand_path method with __FILE__ and is made from the gemspec file" do
let(:filename) { "#{project_root}/bar.gemspec" }
let(:source) { "require File.expand_path('../lib/foo/version', __FILE__)" }
let(:source) { "File.expand_path('../lib/foo/version', __FILE__)" }

it "does not register an offense" do
expect_no_offenses(<<~RUBY, filename)
Expand All @@ -214,9 +123,9 @@
end
end

context "when `require` call uses File#expand_path method with __dir__ but lies inside lib/" do
context "when use of File#expand_path method with __dir__ but lies inside lib/" do
let(:filename) { "#{project_root}/lib/foo/bar/baz/qux.rb" }
let(:source) { "require File.expand_path('../../foo', __dir__)" }
let(:source) { "File.expand_path('../../foo', __dir__)" }

it "does not register an offense" do
expect_no_offenses(<<~RUBY, filename)
Expand All @@ -225,9 +134,9 @@
end
end

context "when `require` call uses File#expand_path method with __dir__ and is made from the gemspec file" do
context "when use of File#expand_path method with __dir__ and is made from the gemspec file" do
let(:filename) { "#{project_root}/qux.gemspec" }
let(:source) { "require File.expand_path('lib/qux/version', __dir__)" }
let(:source) { "File.expand_path('lib/qux/version', __dir__)" }

it "does not register an offense" do
expect_no_offenses(<<~RUBY, filename)
Expand All @@ -236,9 +145,9 @@
end
end

context "when the `require` call uses File#dirname with __FILE__ but lies inside tests/" do
context "when use of File#dirname with __FILE__ but lies inside tests/" do
let(:filename) { "#{project_root}/tests/foo/bar_spec.rb" }
let(:source) { "require File.dirname(__FILE__) + '/../lib/bar'" }
let(:source) { "File.dirname(__FILE__) + '/../lib/bar'" }

it "does not register an offense" do
expect_no_offenses(<<~RUBY, filename)
Expand All @@ -247,9 +156,9 @@
end
end

context "when the `require` call uses File#dirname with __FILE__ but lies inside lib/" do
context "when use of File#dirname with __FILE__ but lies inside lib/" do
let(:filename) { "#{project_root}/lib/baz/qux.rb" }
let(:source) { "require File.dirname(__FILE__) + '/../baz'" }
let(:source) { "File.dirname(__FILE__) + '/../baz'" }

it "does not register an offense" do
expect_no_offenses(<<~RUBY, filename)
Expand All @@ -258,9 +167,9 @@
end
end

context "when the `require` call uses File#dirname with __FILE__ and is made from the gemspec file" do
context "when use of File#dirname with __FILE__ and is made from the gemspec file" do
let(:filename) { "#{project_root}/bar.gemspec" }
let(:source) { "require File.dirname(__FILE__) + '/../lib/bar/version'" }
let(:source) { "File.dirname(__FILE__) + '/../lib/bar/version'" }

it "does not register an offense" do
expect_no_offenses(<<~RUBY, filename)
Expand All @@ -269,9 +178,9 @@
end
end

context "when the `require` call uses File#dirname with __dir__ but lies inside spec/" do
context "when use of File#dirname with __dir__ but lies inside spec/" do
let(:filename) { "#{project_root}/spec/foo/bar_spec.rb" }
let(:source) { "require File.dirname(__dir__) + '/../lib/bar'" }
let(:source) { "File.dirname(__dir__) + '/../lib/bar'" }

it "does not register an offense" do
expect_no_offenses(<<~RUBY, filename)
Expand All @@ -280,9 +189,9 @@
end
end

context "when the `require` call uses File#dirname with __dir__ but lies inside lib/" do
context "when use of File#dirname with __dir__ but lies inside lib/" do
let(:filename) { "#{project_root}/lib/baz/qux.rb" }
let(:source) { "require File.dirname(__dir__) + '/../baz'" }
let(:source) { "File.dirname(__dir__) + '/../baz'" }

it "does not register an offense" do
expect_no_offenses(<<~RUBY, filename)
Expand All @@ -291,9 +200,9 @@
end
end

context "when the `require` call uses File#dirname with __dir__ and is made from the gemspec file" do
context "when use of File#dirname with __dir__ and is made from the gemspec file" do
let(:filename) { "#{project_root}/baz.gemspec" }
let(:source) { "require File.dirname(__dir__) + '/lib/baz/version'" }
let(:source) { "File.dirname(__dir__) + '/lib/baz/version'" }

it "does not register an offense" do
expect_no_offenses(<<~RUBY, filename)
Expand Down