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 for internal requires #521

Merged
merged 1 commit into from
Sep 28, 2024

Conversation

tagliala
Copy link
Contributor

@tagliala tagliala commented Sep 22, 2024

require_relative is preferred over require for files within the same project because it uses paths relative to the current file, making code more portable and less dependent on the load path.

This change updates internal requires to use require_relative for consistency, performance, and improved portability.

Retain require for dynamic paths in lib/haml_lint.rb because they use absolute paths.

Refs:


Hello, this should also provide a minor performance improvement in loading time because require_relative is O(1), but please confirm my findings

./bin/haml-lint --version (with changes to prefer the current library) appears to be 7%/4.5% faster on M1 Pro, according to Ruby version and architecture

# 3.3.5 x64    `real` (avg of several runs)
`main`:        0m1.625s
`this branch`: 0m1.546s

# 3.3.3 arm64  `real` (avg of several runs)
`main`:        0m0.933s
`this branch`: 0m0.883s

If this is going to be accepted, I'll do the same for slim-lint

`require_relative` is preferred over `require` for files within the same
project because it uses paths relative to the current file, making code
more portable and less dependent on the load path.

This change updates internal requires to use `require_relative` for
consistency, performance, and improved portability.

Retain require for dynamic paths in `lib/haml_lint.rb` because they use
absolute paths.

Refs:
- rubocop/rubocop#8748
@tagliala tagliala force-pushed the chore/prefer-require-relative branch from 93ce339 to 7e3fb4e Compare September 28, 2024 05:44
Copy link
Owner

@sds sds left a comment

Choose a reason for hiding this comment

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

Supportive, just confused about the term "relative" here.

require 'haml_lint/version'
require 'haml_lint/version_comparer'
require 'haml_lint/severity'
require_relative 'haml_lint/constants'
Copy link
Owner

Choose a reason for hiding this comment

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

Why would this not be ./constants? (Just curious)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi,

no because haml_lint is in lib, and constants is in lib/haml_lint/constants.rb, so the relative path does include haml_lint

lib/
├── haml_lint/
│   └── constants.rb
└── haml_lint.rb

This is a good reference in a (former) ruby core gem: https://github.com/ruby/logger/blob/d1d704a6336a82903011336a0116012e9aa4a504/lib/logger.rb#L17-L21

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some other reference in Ruby core that are not linked in this PR:

Copy link
Owner

Choose a reason for hiding this comment

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

Reading from the GitHub app on my phone was throwing me off. Pathing makes sense 👍

@sds sds merged commit f111531 into sds:main Sep 28, 2024
35 checks passed
@sds
Copy link
Owner

sds commented Sep 28, 2024

Thanks. Reads odd to me for a "relative" path to not include a leading ./, but no objections otherwise.

@tagliala tagliala deleted the chore/prefer-require-relative branch September 28, 2024 17:19
@tagliala
Copy link
Contributor Author

Thanks, I'm going to do the same for slim_lint

tagliala added a commit to tagliala/slim-lint that referenced this pull request Sep 28, 2024
`require_relative` is preferred over `require` for files within the same
project because it uses paths relative to the current file, making code
more portable and less dependent on the load path.

This change updates internal requires to use `require_relative` for
consistency, performance, and improved portability.

Retain `require` for dynamic paths in `lib/slim_lint.rb` because they
use absolute paths.

Refs:
- rubocop/rubocop#8748
- sds/haml-lint#521
@MaxLap
Copy link
Contributor

MaxLap commented Sep 28, 2024

Thanks. Reads odd to me for a "relative" path to not include a leading ./, but no objections otherwise.

In case it helps, the way I see it, in general, every path is "relative" unless it starts with a /. The question is relative to what. If it starts with ./, it's relative to current directory (current being unusual in the contect, ~/ is relative home directory and otherwise its relative to some chosen default.

In the case of requires within a gem, what is nearly always desired is relative to the current file (requirrelative to gem's root could make sense too, but that would be extra work to figure out with little benefit) . This is what require_relative does. It is better than using require with a special path because its less error prone and can be faster by not having to do extra processing on the string.

tagliala added a commit to tagliala/slim-lint that referenced this pull request Sep 28, 2024
`require_relative` is preferred over `require` for files within the same
project because it uses paths relative to the current file, making code
more portable and less dependent on the load path.

This change updates internal requires to use `require_relative` for
consistency, performance, and improved portability.

Retain `require` for dynamic paths in `lib/slim_lint.rb` because they
use absolute paths.

Refs:
- rubocop/rubocop#8748
- sds/haml-lint#521
sds pushed a commit to sds/slim-lint that referenced this pull request Sep 28, 2024
`require_relative` is preferred over `require` for files within the same
project because it uses paths relative to the current file, making code
more portable and less dependent on the load path.

This change updates internal requires to use `require_relative` for
consistency, performance, and improved portability.

Retain `require` for dynamic paths in `lib/slim_lint.rb` because they
use absolute paths.

Refs:
- rubocop/rubocop#8748
- sds/haml-lint#521
@tagliala
Copy link
Contributor Author

Can you confirm any actual speed-up?

I'm having mixed results, it appears to be no differences between the two versions in a third-party application after a few runs

@sds
Copy link
Owner

sds commented Sep 28, 2024

From the rubocop thread you linked it sounds like this potentially has more of an impact on Windows machines?

But perhaps there's a reason that issue was never closed.

@tagliala
Copy link
Contributor Author

tagliala commented Sep 29, 2024

I've tested the released gems also on windows and unfortunately, at least on my environment, there are no performance differences

require "haml_lint"

haml-lint 0.58.0
Average: 2.0339336999997615

haml-lint 0.59.0
Average: 2.029540899999847

bundle exec haml-lint --version

haml-lint 0.58.0
Average: 2.8111768999995546

haml-lint 0.59.0
Average: 2.805723960000614

haml-lint --version

haml-lint 0.58.0
Average: 2.2467118700020365

haml-lint 0.59.0
Average: 2.2358569299976807

In any case, performance would have been only a nice side effect, there are other reasons to switch to require_relative:

@sds
Copy link
Owner

sds commented Sep 29, 2024

Thank you for performing the benchmarks. Agree even if there's no significant difference it's still worth keeping the use of require_relative.

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.

3 participants