Introduce explicit rails server handler option#32058
Conversation
|
r? @schneems (@rails-bot has picked a reviewer for you, use r? to override) |
|
The previous error was: |
kaspth
left a comment
kaspth
left a comment
There was a problem hiding this comment.
👍 to adding the argument validation. But I don't understand why it follows that we should turn the server positional arg into an option?
There was a problem hiding this comment.
We're on Ruby 2.3 now, so lets use the squiggly heredoc <<~
There was a problem hiding this comment.
Think this might be clearer as:
suggestions = HANDLERS.sort_by { |h| levenshstein_distance(handler, h) }.map(&:inspect)Don't think the first(2) makes sense since we use first and second below — there's only 8 things in the array, we aren't saving anything with it anyway.
There was a problem hiding this comment.
The only subtle difference with .map(&:inspect) is that the message contains double quotes instead of single ones.
There was a problem hiding this comment.
I think we might prefer %w( … )
There was a problem hiding this comment.
I know Rack uses handler, but it reads a tad too generic here for me — especially if the shorthand is -x. How about -r for runner? E.g. the thing to run the rails server.
There was a problem hiding this comment.
--using? Runner seems particularly problematic given rails runner.
There was a problem hiding this comment.
@matthewd yeah, definitely thought that -r is on sketchy ground because of that clash. I like --using. 👍
There was a problem hiding this comment.
--using sounds good to me too.
There was a problem hiding this comment.
Don't think attr_reader :output and @output makes sense. We can just do:
assert_match(/Could not find handler 'tink'. Maybe you meant 'thin' or 'cgi'/, run_command(["tink"]))Then it'll match our test/application/test_runner_test.rb tests too.
There was a problem hiding this comment.
How deterministic is it that cgi will be the other option here? I don't think we'd want intermittent test failures here.
There was a problem hiding this comment.
The handlers list is hard-coded, as we don't have a clean way to query the handlers from rack/lib/rack/handler.rb. If the order of the HANDLERS doesn't change often, the test should be stable.
|
@kaspth we removed the positional parameter for (Specifically, given that we can only load a handler that's available in the Gemfile, and that we automatically load the first handler that's available in the Gemfile, it seems pretty rare to me for people to have cause to name a handler at all.) |
There was a problem hiding this comment.
AFAICS this could end up suggesting exactly what they typed, which seems not-good. If handler is in HANDLERS, maybe we should instead suggest that they're missing a Gemfile entry.
|
@matthewd sure, but that was for the environment, no? I thought Anyway, you're right that passing it manually is probably a minority case, so lets go with the deprecation — and |
gsamokovarov
force-pushed
the
rails-server-x-option
branch
from
ccc1d5b to
c5b01cb
Compare
|
I have changed the option to |
gsamokovarov
force-pushed
the
rails-server-x-option
branch
from
c5b01cb to
a80f003
Compare
|
This is probably for another time, but… I just realized that now we're on 2.3+ we could leverage the unless HANDLERS.include?(options[:using])
DidYouMean::SpellChecker.new(HANDLERS).correct(options[:using])
endWe could also remove Rails' Levenshtein distance method from other places. |
There was a problem hiding this comment.
Doing argv validation seems like it should a concern of the command and not the server itself. Both include Rails::Command::Behavior and rails server --help seem to allude as much.
Could we move it to the command if we do this?
capturing_missing_handler do
server.start
endThere was a problem hiding this comment.
Tried that as well – it's still messy because levenshtein_distance is a private class method:
def suggest_rack_server(handler)
if handler.in?(HANDLERS)
<<~MSG
Could not load server "#{handler}". Maybe you need the add it to the Gemfile?
gem "#{handler}"
Run `rails server --help` for more options.
MSG
else
suggestions = HANDLERS.sort_by { |h| self.class.send(:levenshtein_distance, handler, h) }.map(&:inspect)
<<~MSG
Could not find server "#{handler}". Maybe you meant #{suggestions.first} or #{suggestions.second}?
Run `rails server --help` for more options.
MSG
end
endThere was a problem hiding this comment.
I'm guessing the stopgap to inlining the HandlerSuggestion module in the command is this unless. But I don't fully understand why we need it? Isn't it fine for the server to say Exiting in the off case that a handler is missing?
There was a problem hiding this comment.
Actually, the stopgap is the levenstein_distance method being private and defined in the Rails::Command::Behavior module which carries command specific helpers. Decided to at least isolate it. I'd love to revisit the levenshtein_distance usage or swap it for the algo in DidYouMean, but I'd like to do it in a follow-up change, as it will have to touch the generator commands as well.
The guard here is for my personal aesthetic reasons. 😅 This is the message with the trailing Exiting:
Could not load server "unicorn". Maybe you need the add it to the Gemfile?
gem "unicorn"
Run `rails server --help` for more options.
Exiting
The Exiting line seems redundant to me, especially with "Run rails server --help for more options." as a final suggestion. Maybe the user mistyped an option and triggered this?
Can an option be to complicate the existing guard a bit and move the missing_handler check there?
# ...
ensure
# The '-h' option calls exit before @options is set.
# If we call 'options' with it unset, we get double help banners.
puts "Exiting" unless @options && options[:daemonize] || missing_handler
endThere was a problem hiding this comment.
Personally I'd probably extract the message into a method like below, but it's probably fine like this :)
@server = deprecate_positional_using(using) || options[:using]There was a problem hiding this comment.
👍 I think I can extract it in a separate method.
There was a problem hiding this comment.
It still needs the squish; deprecation messages should not be hard-wrapped on output.
There was a problem hiding this comment.
We should make sure to only rescue a LoadError for "rack/handler/#{options[:using]}"; if something else is wrong, the original exception will do a better job of describing the issue.
There was a problem hiding this comment.
Rack::Handler.get can raise either LoadError or NameError, if the Puma constant does not exist after the rack/handler/puma require, for example. I think I can override the Rack::Server#server method to isolate the error catch.
There was a problem hiding this comment.
Can not deal with this by explicitly calling Rack::Handler.get before starting server?
I am also concerned that other errors by LoadError will be wrapped by this.
There was a problem hiding this comment.
We can, I'll do the changes accordingly.
gsamokovarov
force-pushed
the
rails-server-x-option
branch
from
a80f003 to
48b0796
Compare
|
@kaspth, I think I'll be able to get the printing out of the As for moving the handler suggestion logic entirely into the |
There was a problem hiding this comment.
I'm curious. Is the user using the server option so much that mistyped check is needed?
I understand that mistakenly specify server as env. But will not it solve the necessity of option to specify server?
There was a problem hiding this comment.
@y-yagi doubt it's used often. This tries to give you more context on what actually happens when you pass a positional argument. I'm setting the suggestions here by suggestion (rim-shot). 😅 Anyway, I think it's quite easy to setup the suggestions and we can follow the example in more command options that accept a fixed set of values.
gsamokovarov
force-pushed
the
rails-server-x-option
branch
from
48b0796 to
d2c9309
Compare
|
Friends, I have moved the printing out of |
There was a problem hiding this comment.
I can wrap this in a capturing method.
There was a problem hiding this comment.
Same here. The purpose of the Exiting message is to show up, for example, when you CTRL-C/terminate a running server.
There was a problem hiding this comment.
url can be nil here, if we're not on Puma.
kaspth
left a comment
kaspth
left a comment
There was a problem hiding this comment.
I really like where you took this in this round, Genadi! Hadn't thought of extracting all the printing to the command.
Also don't be afraid to say if you've had enough review rounds and would rather focus on wrapping it up. Then we'll find a way to cut it.
I've been using this case to ease back into the command infrastructure and what exactly the responsibility of a command should be. So if it feels like it's playing around a little bit — that's because it is! 😄
There was a problem hiding this comment.
How about:
def serveable? # :nodoc:
server
true
rescue LoadError, NameError
false
endThen in the command:
unless server.serveable?
say HandlerSuggestion.message(using) # We ought to use `say` now that we're in the command.
return
endHell, we could even get server.start to yield to a finalizer block when it exits. Then it'd be:
server.start do |exited_successfully|
if exited_successfully
say "Exiting" unless options[:daemon]
else
say HandlerSuggestion.message(using)
end
endThere was a problem hiding this comment.
Ugh. The last suggestion won't work because of print_boot_information. Maybe there's still something useful we can dig out of the suggestion though? 😊
There was a problem hiding this comment.
Kasper, I have tried something similar in the last round. I wasn't able to extract all the params out of print_boot_information, because using can be nil, or empty if we coerce it, but that still won't give us a server name to print.
There was a problem hiding this comment.
If this being private is what stops us from removing this HandlerSuggestion then lets make it public. It's marked as :doc: anyway, so it's already public API.
There was a problem hiding this comment.
It's also a class method, so I'd have to call it through self.class. We go through lots of hoops to use it now, so I'll extract it in a Rails::Command::Spellchecker module as we already blew up the PR a bit. 😅
There was a problem hiding this comment.
Shouldn't this just use using?
There was a problem hiding this comment.
=> Booting Puma, where using is usually puma. But yeah, we can save this argument pass.
There was a problem hiding this comment.
Let's let the server build this URL in a nodoc'ed served_url, so we can do server.served_url below.
Then we can keep use_puma?
There was a problem hiding this comment.
I think it would be better and clearer to do this than the @_using construct:
super
deprecate_positional_using if @using
@using ||= options[:using]
@log_stdout = …
|
Don't worry Kasper I'm having fun as well. 😀 |
There was a problem hiding this comment.
This could technically be a backward incompatible change, but:
- I don't think we should list
Rails::Serverin the documentation at all. - This change aims Rails 6, so it should be okay.
There was a problem hiding this comment.
- We have listed it in the past, so it's technically public API.
- Just because it's a major doesn't mean we should just make undue backwards incompatible changes willy-nilly.
I am learning towards the Rails::Server published as public API being a mistake and not being intended. I don't think any of the other Railties command helpers are public.
What does a GitHub search for Rails::Server say? It might corroborate the above 😊
There was a problem hiding this comment.
If we find there's no real usage of Rails::Server on GitHub, I'm 👍 on # :nodoc: (but do add the space so it matches the command nodoc 🤓)
kaspth
left a comment
kaspth
left a comment
There was a problem hiding this comment.
Really like how this turned out 👍
Feels right that the command handles the error printing and the server is more focused on its own boot.
Some minor nits left, then I'll merge.
There was a problem hiding this comment.
Let's put a newline after this.
There was a problem hiding this comment.
For another time: I could totally see us enriching Thor's argument/option with a :suggestions to get messages like this for free. E.g.:
class_option :using, aliases: "-u", type: :string, suggestions: RACK_SERVERS,
desc: "Specifies the Rack server used to run the application (thin/puma/webrick).", banner: :nameThere was a problem hiding this comment.
👍, dig that we got rid of these comments.
There was a problem hiding this comment.
If we find there's no real usage of Rails::Server on GitHub, I'm 👍 on # :nodoc: (but do add the space so it matches the command nodoc 🤓)
|
Oh, needs a commit squash and rebase too! Normally we'd want 1 commit, but I'll grant you a commit for the spellchecker change first, then one for the server changes after that if you want. 1 commit is fine too 👍 |
gsamokovarov
force-pushed
the
rails-server-x-option
branch
2 times, most recently
from
245c603 to
dd8b60e
Compare
I mistype `rails server production` instead of `rails server -e production` expecting to lunch a server in the production environment all the time. However, the signature of `rails server --help` is: ``` Usage: rails server [puma, thin etc] [options] ``` This means that the `production` argument is being interpreted as a Rack server handler like Puma, Thin or Unicorn. Should we argue for the `rails server production`? I'm not sure of the reasons, but the `rails console production` behavior was deprecated in: rails#29358, so parity with the existing `rails console production` usage may not hold anymore. In any case, this PR introduces an explicit option for the Rack servers configuration. The option is called `--using` (or `-u` for short) to avoid the `rails server --server` tantrum. The new interface of `rails server` is: ``` Usage: rails server [using] [options] Options: -p, [--port=port] # Runs Rails on the specified port - defaults to 3000. -b, [--binding=IP] # Binds Rails to the specified IP - defaults to 'localhost' in development and '0.0.0.0' in other environments'. -c, [--config=file] # Uses a custom rackup configuration. # Default: config.ru -d, [--daemon], [--no-daemon] # Runs server as a Daemon. -e, [--environment=name] # Specifies the environment to run this server under (development/test/production). -u, [--using=name] # Specifies the Rack server used to run the application (thin/puma/webrick). -P, [--pid=PID] # Specifies the PID file. # Default: tmp/pids/server.pid -C, [--dev-caching], [--no-dev-caching] # Specifies whether to perform caching in development. [--early-hints], [--no-early-hints] # Enables HTTP/2 early hints. ``` As a bonus, if you mistype the server to use, you'll get an auto-correction message: ``` $ rails s tin Could not find handler "tin". Maybe you meant "thin" or "cgi"? Run `rails server --help` for more options. ```
gsamokovarov
force-pushed
the
rails-server-x-option
branch
from
dd8b60e to
a951729
Compare
|
Kasper, I have searched for
This means that we cannot match |
|
Fine with me! Really glad how this turned out 👏 |
|
Thanks for the review! 🙇♂️ |
|
Did some work on the server command a0061d2...1d97b8f and realized I forgot to backport this to 5-2-stable 🤭 |
- Because just passing the server argument to this command is deprecated in #32058
|
Recover all my page |
7 participants
I mistype
rails server productioninstead ofrails server -e productionexpecting to lunch a server in the production environmentall the time. However, the signature of
rails server --helpis:This means that the
productionargument is being interpreted as a Rackserver handler like Puma, Thin or Unicorn.
Should we argue for the
rails server production? I'm not sure of thereasons, but the
rails console productionbehavior was deprecated in:#29358, so parity with the existing
rails console productionusage may not hold anymore.In any case, this PR introduces an explicit option for the Rack servers
configuration. I call them
--handlers(or-xfor short, as-hisalready as the shortcut for
--help) to avoid therails server --servertantrum.The new interface of
rails serveris:As a bonus, if you mistype the handler, you'll get an auto-correction
message: