Skip to content

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

Merged
merged 1 commit into from
Apr 9, 2023

Conversation

zzak
Copy link
Member

@zzak zzak commented Apr 7, 2023

This PR introduces a new (private) constant under Rails:

  • Rails::Rackup::Server which is used by the bin/rails server command.

In order to fix this test:

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

@rails-bot rails-bot bot added the railties label Apr 7, 2023
@zzak zzak force-pushed the rack-3-rackup-server branch from 5547c98 to dde2183 Compare April 7, 2023 09:43
Copy link
Member

@skipkayhil skipkayhil left a comment

Choose a reason for hiding this comment

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

Nice!

# :enddoc:

module Rails
module Rack
Copy link
Contributor

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:

  1. Anyone who writes Rack:: within the Rails namespace will get this module instead of ::Rack.
  2. Going forward, ::Rackup::Server is the proper name.

Copy link
Member Author

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.

Copy link
Member Author

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>
@zzak zzak force-pushed the rack-3-rackup-server branch from dde2183 to 05e9464 Compare April 8, 2023 22:15
@guilleiguaran guilleiguaran merged commit e99aa34 into rails:main Apr 9, 2023
@zzak zzak deleted the rack-3-rackup-server branch April 9, 2023 07:45
@ioquatix
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants