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

Jump to bottom

[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

wants to merge 1 commit into from

Conversation

samyak-jn
Copy link

Hi,

This is with regards to issue #35 for the same I have tweaked the AST so that it checks the usage of File.expand_path and File.dirname used with lib/.
I am not so much proficient with ruby, so please correct me if I'm wrong, thanks! :)

Once, the AST is finalized we can discuss more renaming the cop and etcetera.

@samyak-jn
Tweak the AST so that it checks the usage of File.expand_path used wi…
73210b0 
…th lib

Update tests according to the new AST

Signed-off-by: Samyak Jain <samyak.jn11@gmail.com>
utkarsh2102
utkarsh2102 requested changes Apr 20, 2021
View reviewed changes
Copy link
Owner

@utkarsh2102 utkarsh2102 left a comment

Choose a reason for hiding this comment

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

Hi @samyak-jn,

Thanks for working on this but I am not sure if this is what we need? We should essentially generalize the AST to catch all the "bad" calls and "require" ones were just a subset of those. I don't expect someone to go through the AST in detail unless we have the existing tests passing + we've added more generalized tests, which are passing as well. The AST should then "speak for itself" and maybe someone could then do the final edge-case testing and stuff and do a finish-up of the review/merge. Does that make sense? 😅

Let me know if we're not on the same page or something or if I am simply missing anything?

spec/rubocop/cop/packaging/require_hardcoding_lib_spec.rb
Comment on lines -8 to -44
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
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).

spec/rubocop/cop/packaging/require_hardcoding_lib_spec.rb
Comment on lines -140 to +104
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
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.

lib/rubocop/cop/packaging/require_hardcoding_lib.rb
Comment on lines -68 to +67
return unless require?(node)
return unless File?(node)
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. :)

@samyak-jn
Copy link
Author

Hi @utkarsh2102,

Thanks for working on this but I am not sure if this is what we need? We should essentially generalize the AST to catch all the "bad" calls and "require" ones were just a subset of those.
Let me know if we're not on the same page or something or if I am simply missing anything?

Thanks for quite a verbose explanation, and yes I agree with your point on this.
Before digging deeper I wanted to make sure if I'm proceeding in the right direction, currently, the cop checks just for the require calls and we want this cop to just not specifically check for former but also to check for the relative path being used to lib/ (which basically focus on the implementation of File.dirname and File.expand_path methods), right?

For the very same here's the AST below could you please verify them?


        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?)))
           (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?))}

Thanks for taking your out time for this :)

@utkarsh2102
Copy link
Owner

Oh no, I don't think we want :require in the AST. The AST could be generic as what you have in the PR atm, but that AST should also be able to detect the require calls. So basically, from "require"-specific AST, we want to move towards something generic that'd catch all sorts of calls that use relative path, including the require calls themselves, but the "require" keyword shouldn't be an offense anymore.

@utkarsh2102
Copy link
Owner

utkarsh2102 commented Apr 21, 2021 •
edited

Plus, I guess, the CI should be green, that's the very first thing I'd like to see, heh.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@utkarsh2102 utkarsh2102 utkarsh2102 requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@samyak-jn @utkarsh2102