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

[samyak-jn]

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]

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]

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]

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