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

Use require_relative instead of require with full path #29417

Conversation

bogdanvlviv
Copy link
Contributor

@jeremy suggested converting require with a full path to require_relative. See #29176 (review)

After brief discuss with @fxn we decided to do it.

There are four cases i'm not sure about using require_relative:

Related to #29176

r? @fxn, @jeremy

Copy link
Member

@matthewd matthewd left a comment

Choose a reason for hiding this comment

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

I don't like the redundancy introduced by some of these (ar_tests, COMPONENT_ROOT).

I think I also prefer the explicit ./ prefix that you're removing from some lines here, but that's just a minor preference.

@fxn fxn self-requested a review June 12, 2017 07:31
Copy link
Member

@fxn fxn left a comment

Choose a reason for hiding this comment

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

I am fine with the changes.

ar_tests is the only one where I don't see quite the benefit. Also unsure about

require "cases/helper"

On the other hand COMPONENT_ROOT is a constant set as

COMPONENT_ROOT = File.expand_path("..", __dir__)

in the bin/test of every Rails component.

Regarding "./", I am totally for removing them because it is redundant. I never write leading dots in relative paths unless needed. For example, I do not write vim ./foo.rb, I always say vim foo.rb if foo.rb is in the current directory. When you do not have the cwd in $PATH then you use a leading dot to run an executable, but that is because it just does not work without it.

Summary, require_relative takes a path and resolves it relative to __dir__, the dot is adding no information there.

@@ -10,8 +10,8 @@ def setup
database_config = { "adapter" => "postgresql", "database" => "activerecord_unittest" }
ar_tests = File.expand_path("../../../activerecord/test", __dir__)
if Dir.exist?(ar_tests)
require File.join(ar_tests, "config")
require File.join(ar_tests, "support/config")
Copy link
Member

Choose a reason for hiding this comment

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

This one is clear to me, so I'd avoid change it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree we should leave as it is.

@bogdanvlviv bogdanvlviv force-pushed the use-require_relative-instead-of_require-with-full-path branch from 0fe493a to 19866ae Compare June 12, 2017 20:14
@fxn
Copy link
Member

fxn commented Jun 14, 2017

Approved over here.

@bogdanvlviv can you rebase please?

@bogdanvlviv bogdanvlviv force-pushed the use-require_relative-instead-of_require-with-full-path branch from 19866ae to 6673cf7 Compare June 14, 2017 09:12
@bogdanvlviv
Copy link
Contributor Author

@fxn Done.

@fxn
Copy link
Member

fxn commented Jun 14, 2017

@rafaelfranca's feedback and mine are addressed (I saw that loading cases/helper is done that way in other places).

@matthewd are you cool with the patch?

@matthewd matthewd merged commit 4915bfe into rails:master Jun 14, 2017
@bogdanvlviv
Copy link
Contributor Author

Thank you all for the review! ❤️

@bogdanvlviv bogdanvlviv deleted the use-require_relative-instead-of_require-with-full-path branch June 14, 2017 18:26
bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Oct 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants