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 #350

Merged

Conversation

tagliala
Copy link
Contributor

@tagliala tagliala commented Sep 19, 2024

As a side note, this change has been approved here by @deivid-rodriguez which is familiar with this issue: activeadmin/arbre#622


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.

Ref:


Before merging:

  • Copy the table printed at the end of the latest benchmark results into the README.md and update this PR
  • If this change merits an update to CHANGELOG.md, add an entry following Keep a Changelog guidelines with semantic versioning

@tagliala tagliala force-pushed the chore/prefer-require-relative branch from e814665 to 40cb797 Compare September 19, 2024 12:17
@tagliala tagliala changed the title Chore/prefer require relative Prefer require_relative for internal requires Sep 19, 2024
Copy link

codecov bot commented Sep 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (ac1231e) to head (c8c11f6).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #350   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          190       190           
  Branches        90        90           
=========================================
  Hits           190       190           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@pboling pboling left a comment

Choose a reason for hiding this comment

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

LGTM

JacobEvelyn
JacobEvelyn previously approved these changes Sep 25, 2024
@JacobEvelyn
Copy link
Member

Would you mind actually adding something to the CHANGELOG.md for this? Maybe like:

**Gem enhancements:**

- Changed internal `require`s to `require_relative` to make code less dependent on the load path [[#350](https://github.com/panorama-ed/memo_wise/pull/350)]

if you fixup that change into the same commit I'll approve and merge!

@JacobEvelyn JacobEvelyn dismissed their stale review September 25, 2024 13:25

see comment

@JacobEvelyn JacobEvelyn mentioned this pull request Sep 25, 2024
2 tasks
`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.

Ref:
- ruby/psych#522
- ruby/logger#20
- ruby/rdoc#658
- panorama-ed#349
- rubocop/rubocop#8748
@tagliala tagliala force-pushed the chore/prefer-require-relative branch from 40cb797 to c8c11f6 Compare September 25, 2024 17:18
@tagliala
Copy link
Contributor Author

tagliala commented Sep 25, 2024

@JacobEvelyn done and force pushed in the same commit

As a side note, for some extreme use cases, this can be considered a breaking change

Ref: varvet/pundit#831 (comment)

@pboling
Copy link
Contributor

pboling commented Sep 25, 2024

It is a potential breaking change for a use case that isn't actually a feature or intended. What he describes is a way to break the gem on purpose with knowledge of the source, so that you can hack a library in a very strange way. It is an interesting thought experiment. Several people have now responded to him on the RuboCop discussion.

IMO it would be extreme to consider this a breaking change, especially since it fixes a real use case, which is benchmarking as this gem does.

@JacobEvelyn
Copy link
Member

Yes I agree with both of these that this should not be considered a breaking change.

@JacobEvelyn JacobEvelyn merged commit ef52a1a into panorama-ed:main Sep 25, 2024
12 checks passed
@tagliala tagliala deleted the chore/prefer-require-relative branch September 25, 2024 17:47
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