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.

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

Prefer require_relative #8748

Open
marcandre opened this issue Sep 19, 2020 · 21 comments
Open

Prefer require_relative #8748

marcandre opened this issue Sep 19, 2020 · 21 comments
Labels
feature request good first issue

Comments

@marcandre
Copy link
Contributor

@marcandre marcandre commented Sep 19, 2020

To load dependencies withing a gem/app, require_relative should always be preferred to require

We should check for any instance of (send nil? require `{:__dir__ :__FILE__}) and raise an offense.

I can't event think of a single instance where require should be called with an interpolated string tbh

@utkarsh2102
Copy link
Member

@utkarsh2102 utkarsh2102 commented Sep 19, 2020

Hi @marcandre 👋🏻

I know this is a bit untraditional (& you might be aware of this already) but there's this cop Packaging/RequireRelativeHardcodingLib which wants people to use require over require_relative only inside tests.
Otherwise, we always want to use require_relative (just not from tests to lib/).

Rationale at https://docs.rubocop.org/rubocop-packaging/cops_packaging.html#require-relative-hardcoding-lib-rationale
This will not only help Debian but a lot of other downstreams, too!

Please let me know what you think?

@tas50
Copy link
Contributor

@tas50 tas50 commented Sep 19, 2020

We've been converting our codebase to use require_relative for some time. It works around some pretty terrible dir walking behavior in rubygems that is particularly slow on Windows where disk IO performance in Ruby is less than ideal. It's been on my todo list for a while to write a rule to do this, but we cranked through it with this script. It also uses if defined?(CLASS) to further avoid rubygems performance issues, so just ignore that bit.

https://gist.github.com/tas50/65682084613887ac7e4a4900cfd1e495

@coding-bunny
Copy link

@coding-bunny coding-bunny commented Sep 22, 2020

What about ruby's autoload ?

@marcandre
Copy link
Contributor Author

@marcandre marcandre commented Sep 22, 2020

What about ruby's autoload ?

Matz has not been persuaded yet to add a autoload_relative feature... https://bugs.ruby-lang.org/issues/15330
Or was that not the question?

@coding-bunny
Copy link

@coding-bunny coding-bunny commented Sep 22, 2020

I didn't mean the autoload_relative feature, simply using autoload for loading stuff relevant to your project and sticking to require purely for gems.

@marcandre
Copy link
Contributor Author

@marcandre marcandre commented Sep 22, 2020

sticking to require purely for gems

Yes

using autoload for loading stuff relevant to your project

Exclusively? Especially without a autoload_relative it doesn't seem like a good idea.

@coding-bunny
Copy link

@coding-bunny coding-bunny commented Sep 22, 2020

I don't see why not?
You define what's being loaded as a symbol and provide the path to it.
This is done once, usually in the module defining your gem, or an initializer and everything is done, Ruby loads everything else.

https://medium.com/rubycademy/the-autoload-method-in-ruby-11fd079562ef writes it our cleanly with examples.
It's how I've been handling loading in pretty much every project and avoids all kind of issues with require vs require_relative, checking paths and what not.

@marcandre
Copy link
Contributor Author

@marcandre marcandre commented Sep 22, 2020

I don't see why not?

  • makes loading order less clear
  • autoload without an absolute path is slow (like require)
  • autoload with an absolute path (e.g. using __dir__) is ugly
  • autoloading a feature that is known to be needed immediately is an extra burden that provides no benefit.
  • in some cases, features are not referenced directly, only indirectly because they register themselves (like cops in Rubocop).

https://medium.com/rubycademy/the-autoload-method-in-ruby-11fd079562ef writes it our cleanly with examples.

...with bad examples using paths like './b.rb'. If we don't already, we should consider a cop that check for require './anything' or require '../anything', it's basically never what should be done.

Please note that the decision of require vs autoload is orthogonal to require vs require_relative.

@coding-bunny
Copy link

@coding-bunny coding-bunny commented Sep 22, 2020

I can definitely support the idea that require should not be used for relative paths or interpolated paths. A Cop for that is something nice to have for sure.

@utkarsh2102
Copy link
Member

@utkarsh2102 utkarsh2102 commented Sep 22, 2020

FWIW, there's already a Packaging/RequireHardcodingLib cop which "almost" checks the same thing and autocorrects the offenses.

Why I say almost is because it only checks for those calls which are mapped to the lib/ directory.

That said, I am sure it can be extended to a more generic cop with a few tweaks. I can put together a PR (or take a stab at it) if you'd like so? 😄

@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Sep 25, 2020

Yeah, that'd be great.

@bbatsov bbatsov added feature request good first issue labels Sep 25, 2020
@terceiro
Copy link

@terceiro terceiro commented Sep 25, 2020

FWIW, for downstream distributors, and for other use cases, require_relative from the test files to "[...]/../lib" is a problem because for example you can't run the tests against the code that is installed system-wide (this is an actual use case in Debian for integration testing of the distribution). This what @utkarsh2102 has been working on, starting with GSOC this year, and now continuing beyond the internship.

In general, I think that require_relative is OK within lib/, but I'm a little dubious about the value of saying that everyone should do it. My personal taste is that I prefer requires to be more explicit, because that makes it more clear what exactly they are referring to.

@mvz
Copy link
Contributor

@mvz mvz commented Sep 26, 2020

@terceiro what do you mean by 'more explicit'? Can you give an example?

@coding-bunny
Copy link

@coding-bunny coding-bunny commented Sep 26, 2020

I'd like to see an example as well, as I have a hard time understanding what is being implied.
Especially for "running tests against code that's installed system-wide", I'd like to see a use-case for this actually, unless you are referring to installed gems/packages in Ruby, in which case they would fall under require

@utkarsh2102
Copy link
Member

@utkarsh2102 utkarsh2102 commented Sep 26, 2020

Hi @coding-bunny,

I'd like to see an example as well, as I have a hard time understanding what is being implied.
Especially for "running tests against code that's installed system-wide", I'd like to see a use-case for this actually, unless you are referring to installed gems/packages in Ruby, in which case they would fall under require

Here's the rationale that might suffice: https://docs.rubocop.org/rubocop-packaging/cops_packaging.html#require-relative-hardcoding-lib-rationale. Please feel free to ask if something's not clear!

@coding-bunny
Copy link

@coding-bunny coding-bunny commented Sep 26, 2020

That doesn't really explain it.
When you use require "foo/bar.rb" (explicitly with the .rb) then for me you are doing something wrong. You're requiring files explicitly to something that should be existing on the $LOAD_PATH which either means you're building something very specific, or you have something messing with the load-path for you such as Bundler or Rubygems, on which you should never count as you don't know what is being used for loading the code.

  • Loading specific files should be done with require_relative : e.g require_relative "#{Rails.root.join("lib/foo/bar.rb")
  • Loading gems and frameworks should be done with require: e.g require "myTool/myClass"

@mvz
Copy link
Contributor

@mvz mvz commented Sep 26, 2020

@coding-bunny the example with .rb is a bit of a red herring. The 'Rationale' section explains the use case you asked for.

@coding-bunny
Copy link

@coding-bunny coding-bunny commented Sep 26, 2020

I'm totally fine with the rubocop-packaging checking that constraint when you're using that for the rationale written out there, but that shouldn't be the standard in normal RuboCop, as Debian is/should not be the standard go to for this or example.

For normal Ruby projects, require_relative is the correct case IMO to load things like library files or loading your code inside the test-suite without mangling the $LOAD_PATH, which is a responsibility of Bundler/Rubygems, not your project/code.

@deivid-rodriguez
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez commented Sep 26, 2020

Just to throw my 2 cents here. I think mostly everyone here agrees that require_relative should be used for requiring "internal code" whereas require should be used for requiring "external code". The problem is where to draw the line of what's internal code and what's external code.

I'd like to make the case that anything outside of the current gem "section" (lib/, test/, gemspec, exe/, and so on) should be considered external code and thus require should be used in that case. One of the reasons for this is to be friendly to OS packagers (not only Debian, but in general), but there is another reason I can think of also due to "gem sections" been potentially "split" upon installation. It's the following: If you're coding a default gem, requiring relatively from exe into lib will not work, because when a default gem is installed, the "folder relativity" between your executable and your lib code is different from the one in the source code repository (lib code of default gems lives in RbConfig::CONFIG["rubylibdir"] whereas executables live in a standard gem structure).

Also, since prettiness has been mentioned, I personally consider things like require_relative "../../lib/foo/bar" pretty ugly.

I think it'd be nice that a require_relative cop would appease the different sensibilities and have this choice of what to consider as "external code" made configurable.

@utkarsh2102
Copy link
Member

@utkarsh2102 utkarsh2102 commented Sep 26, 2020

Hi @coding-bunny,

When you use require "foo/bar.rb" (explicitly with the .rb) then for me you are doing something wrong. You're requiring files explicitly to something that should be existing on the $LOAD_PATH which either means you're building something very specific, or you have something messing with the load-path for you such as Bundler or Rubygems, on which you should never count as you don't know what is being used for loading the code.

* Loading specific files should be done with `require_relative`
    : e.g `require_relative "#{Rails.root.join("lib/foo/bar.rb")`

* Loading gems and frameworks should be done with `require`
    : e.g `require "myTool/myClass"`

Eeeks! That's certainly not what's being implied here. The whole idea of providing that as an example was -
Some people, I've seen, use require and require_relative that way (that is, using .rb). And the point of the Packaging extension is not to flag such things but to just say that "don't use require_relative path from tests to lib/". That's all!
Of course, requiring with .rb is not something someone would want to do but people do and it's not Packaging extension's job to tell them that is wrong or whatever :)
I suppose that's another potential cop for rubocop instead?

But yes, I think it's better I just trim off the .rb right away so it doesn't carry the wrong message forward when it's not even intended to! 😄

utkarsh2102 added a commit to utkarsh2102/rubocop-packaging that referenced this issue Sep 26, 2020
This is based on the comment and the discussion
of #rubocop/rubocop/issues/8748#issuecomment-699478216

Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>
utkarsh2102 added a commit to rubocop/packaging-style-guide that referenced this issue Sep 26, 2020
This is based on the comment and the discussion
of #rubocop/rubocop/issues/8748#issuecomment-699478216

Signed-off-by: Utkarsh Gupta <utkarsh@debian.org>
@rrosenblum
Copy link
Contributor

@rrosenblum rrosenblum commented Oct 21, 2020

It's been a while since I looked into require vs require_relative. There used to be an issue with mixing require and require_relative for paths that lived under a symlink. The issue was that the path to the file could be resolved in more than one way which would cause Ruby to require the same file twice. It looks like this issue has already been fixed so there is nothing to worry about.

This may help to provide some historical context on decisions to use require vs require_relative. https://bugs.ruby-lang.org/issues/13695

magneland added a commit to magneland/github-changelog-generator that referenced this issue Mar 26, 2021
Allows `bin/github_changelog_generator` to be invoked from anywhere.
This facilitates easier ad hoc testing locally.

This commit leaves specs unchanged on purpose.

See: rubocop/rubocop#8748
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request good first issue
Projects
None yet
Development

No branches or pull requests

9 participants