[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

samyak-jn · 2021-04-20T17:25:54Z

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.

…th lib Update tests according to the new AST Signed-off-by: Samyak Jain <[email protected]>

utkarsh2102

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?

utkarsh2102 · 2021-04-20T17:32:24Z

spec/rubocop/cop/packaging/require_hardcoding_lib_spec.rb

-  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


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).

utkarsh2102 · 2021-04-20T17:32:51Z

spec/rubocop/cop/packaging/require_hardcoding_lib_spec.rb

-  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


utkarsh2102 · 2021-04-20T17:34:19Z

lib/rubocop/cop/packaging/require_hardcoding_lib.rb

-          return unless require?(node)
+          return unless File?(node)


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?

Yes, of course, thanks for PTO. :)

samyak-jn · 2021-04-21T18:48:35Z

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 · 2021-04-21T18:52:53Z

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 · 2021-04-21T18:54:07Z

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

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 <[email protected]>

utkarsh2102 requested changes Apr 20, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[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

[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

samyak-jn commented Apr 20, 2021

utkarsh2102 left a comment

utkarsh2102 Apr 20, 2021

utkarsh2102 Apr 20, 2021

utkarsh2102 Apr 20, 2021

samyak-jn Apr 21, 2021

samyak-jn commented Apr 21, 2021

utkarsh2102 commented Apr 21, 2021

utkarsh2102 commented Apr 21, 2021 •

edited

Loading

[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

Are you sure you want to change the base?

[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

Conversation

samyak-jn commented Apr 20, 2021

utkarsh2102 left a comment

Choose a reason for hiding this comment

utkarsh2102 Apr 20, 2021

Choose a reason for hiding this comment

utkarsh2102 Apr 20, 2021

Choose a reason for hiding this comment

utkarsh2102 Apr 20, 2021

Choose a reason for hiding this comment

samyak-jn Apr 21, 2021

Choose a reason for hiding this comment

samyak-jn commented Apr 21, 2021

utkarsh2102 commented Apr 21, 2021

utkarsh2102 commented Apr 21, 2021 • edited Loading

utkarsh2102 commented Apr 21, 2021 •

edited

Loading