-
Notifications
You must be signed in to change notification settings - Fork 21.9k
Fix Rack::Deprecation warning which causes this test to fail #47885
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
5547c98
to
dde2183
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.
Nice!
railties/lib/rails/rack/server.rb
Outdated
# :enddoc: | ||
|
||
module Rails | ||
module Rack |
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.
I think you should consider renaming this module to Rackup
, two reasons:
- Anyone who writes
Rack::
within the Rails namespace will get this module instead of::Rack
. - Going forward,
::Rackup::Server
is the proper name.
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.
Addressed (2) in 05e9464.
For (1) the same will be true for Rackup::
inside the Rails namespace. There only thing we could do is introduce an entirely new namespace for the rackup server constant. Personally I prefer the current approach, but happy to adjust if anyone feels strongly about the name.
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.
Also my reasoning for the original name is because there is already a rails/rack/logger
, so putting our Rack stuff near together made sense.
https://github.com/rails/rails/blob/main/railties/lib/rails/rack/logger.rb
…ver` command. ``` Failure: Rails::Command::HelpIntegrationTest#test_prints_help_via_`X:help`_command_when_running_`X`_and_`X:X`_command_is_not_defined [/Users/zzak/code/rails/railties/t est/command/help_integration_test.rb:33]: --- expected +++ actual @@ -1,4 +1,5 @@ -"Commands: +"Rack::Server is deprecated and replaced by Rackup::Server +Commands: bin/rails dev:cache # Toggle development mode caching on/off bin/rails dev:help [COMMAND] # Describe available commands or one specific... ``` From @ioquatix: > Yeah you need to load `rackup/server` and if you get `LoadError` fall back to `Rack::Server`. This PR implements that paraphrasing, and resolves this deprecation: ``` Rack::Server is deprecated and replaced by Rackup::Server ``` Example build: https://buildkite.com/rails/rails/builds/95468#0187559d-3190-4e6f-8ac5-65042b2a5412/1334-1342 Co-authored-by: Samuel Williams <samuel.williams@oriontransfer.co.nz>
dde2183
to
05e9464
Compare
Thanks! |
This PR introduces a new (private) constant under Rails:
Rails::Rackup::Server
which is used by thebin/rails server
command.In order to fix this test:
From @ioquatix:
This PR implements that paraphrasing, and resolves this deprecation:
Example build:
https://buildkite.com/rails/rails/builds/95468#0187559d-3190-4e6f-8ac5-65042b2a5412/1334-1342