Fixup code for use with rackup gem (may be used with rack 3) (#3061)#10
Fixup code for use with rackup gem (may be used with rack 3) (#3061)#10MitchLewis930 wants to merge 1 commit intopr_060_beforefrom
Conversation
* Fixup code for use with rackup gem (may be used with rack 3) * Update rack_default.rb * Update puma.rb, use `include` instead of `module_eval` * Changes per comments
There was a problem hiding this comment.
Pull request overview
This PR refactors the Puma codebase to support both the legacy Rack 2 handler pattern and the newer rackup gem (used with Rack 3), where the rackup functionality was extracted into a separate gem.
Changes:
- Introduced conditional module selection based on environment variable to use either
Rack::HandlerorRackup::Handler - Refactored handler code to use a shared
Puma::RackHandlermodule included in bothRack::Handler::PumaandRackup::Handler::Puma - Updated Gemfile to conditionally require the rackup gem when not using Rack 2
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/test_rack_handler.rb | Wraps tests in TestRackUp module and uses RACK_HANDLER_MOD constant to reference the appropriate handler module based on environment |
| lib/rack/handler/puma.rb | Extracts shared handler logic into Puma::RackHandler module and conditionally defines handlers for both Rack 2 and rackup gem |
| lib/puma/rack_default.rb | Adds conditional logic to define default handler for both Rack 2 and rackup gem scenarios |
| Gemfile | Conditionally includes rackup gem dependency when not testing with Rack 2 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| :Silent => false | ||
| } | ||
|
|
||
| def config(app, options = {}) |
There was a problem hiding this comment.
Methods in the Puma::RackHandler module should be defined with self. prefix or use module_function to ensure they work correctly when included via class << self. Currently these are instance methods, but they're being included into singleton classes and called as class methods. Consider using module_function :config, :run, :valid_options, :set_host_port_to_config after the method definitions to make them work properly as both instance and module methods.
| end | ||
| end | ||
| else | ||
| raise "You must install the rackup gem when using Rack 3" |
There was a problem hiding this comment.
The error message should be more actionable by specifying how to install the rackup gem. Consider changing to: 'You must install the rackup gem when using Rack 3. Run: gem install rackup'
| raise "You must install the rackup gem when using Rack 3" | |
| raise "You must install the rackup gem when using Rack 3. Run: gem install rackup" |
| end | ||
| end | ||
| else | ||
| raise "Rack 3 must be used with the Rackup gem" |
There was a problem hiding this comment.
The error message should be more actionable and consistent with the error message in lib/rack/handler/puma.rb. Consider changing to: 'Rack 3 must be used with the rackup gem. Run: gem install rackup'
| raise "Rack 3 must be used with the Rackup gem" | |
| raise "Rack 3 must be used with the rackup gem. Run: gem install rackup" |
2 participants
PR_060