-
Notifications
You must be signed in to change notification settings - Fork 0
Eager loads rails projects when running shoryuken #108
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
Conversation
7dc8e88 to
21c3d2f
Compare
21c3d2f to
5ef328b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR ensures that Rails eager loading is enabled when running Shoryuken so background workers are discovered correctly and corrects a RuboCop configuration.
- Add a Railtie hook (
overwrite_eager_load) to turn onconfig.eager_loadfor Shoryuken processes - Fix the LineLength cop name in
.rubocop.ymlfromMetrics/LineLengthtoLayout/LineLength
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/turtle/railtie.rb | Introduce overwrite_eager_load method and invoke it before initialization when running via Shoryuken |
| .rubocop.yml | Update cop name from Metrics/LineLength to Layout/LineLength |
Comments suppressed due to low confidence (2)
lib/turtle/railtie.rb:40
- [nitpick] The method name
overwrite_eager_loadis generic; consider renaming it to something more descriptive likeenable_eager_load_for_shoryukento clarify its intent.
def overwrite_eager_load
lib/turtle/railtie.rb:40
- There are no existing tests for this new eager load behavior. Adding unit or integration tests to verify that eager_load is toggled only under Shoryuken will prevent regressions.
def overwrite_eager_load
| return unless defined?(::Rails) | ||
| return if ::Rails.application.config.eager_load == true | ||
|
|
||
| Logger.info('Shoryuken and rails detected, overwriting ::Rails.application.config.eager_load to true') |
Copilot
AI
May 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using Logger.info may not integrate with Rails logging. Consider using Rails.logger.info so this message appears in the Rails logs.
| Logger.info('Shoryuken and rails detected, overwriting ::Rails.application.config.eager_load to true') | |
| Rails.logger.info('Shoryuken and rails detected, overwriting ::Rails.application.config.eager_load to true') |
| # https://github.com/ruby-shoryuken/shoryuken/blob/546e4b81afbbacdc7ed6d742a96025be4616f292/lib/shoryuken/default_worker_registry.rb#L15-L28 | ||
| # | ||
| # Therefore the following code is needed. | ||
|
|
Copilot
AI
May 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] This helper method is public by default; marking it as private will prevent unintended external calls and improve encapsulation.
| private |
| # $PROGRAM_NAME returns the name of the script being executed, i.e. "/usr/local/bundle/bin/shoryuken" | ||
| return if $PROGRAM_NAME.to_s.split('/').last.downcase != 'shoryuken' |
Copilot
AI
May 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Relying on $PROGRAM_NAME string matching can be brittle; consider detecting Shoryuken via a dedicated environment variable or CLI flag if available.
| # $PROGRAM_NAME returns the name of the script being executed, i.e. "/usr/local/bundle/bin/shoryuken" | |
| return if $PROGRAM_NAME.to_s.split('/').last.downcase != 'shoryuken' | |
| # Check for the presence of the SHORYUKEN_MODE environment variable | |
| return unless ENV['SHORYUKEN_MODE'] == 'true' |
Uh oh!
There was an error while loading. Please reload this page.