use Thor option parser in server commands parse #26977

y-yagi · 2016-11-05T23:38:13Z

Summary

In master, when start rails server with arguments, it will fail.

$ ./bin/rails -v
Rails 5.1.0.alpha
$ ./bin/rails s -p 3001
/home/yaginuma/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/thor-0.19.1/lib/thor/base.rb:483:in `handle_argument_error': ERROR: "rails server" was called with arguments ["-p", "3001"] (Thor::InvocationError)
Usage: "bin/rails server [options]"
	from /home/yaginuma/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/thor-0.19.1/lib/thor/command.rb:34:in `rescue in run'
	from /home/yaginuma/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/thor-0.19.1/lib/thor/command.rb:21:in `run'
	from /home/yaginuma/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/thor-0.19.1/lib/thor/invocation.rb:126:in `invoke_command'
	from /home/yaginuma/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/thor-0.19.1/lib/thor.rb:359:in `dispatch'
	from /home/yaginuma/program/rails/master_y_yagi/rails/railties/lib/rails/command/base.rb:61:in `perform'
	from /home/yaginuma/program/rails/master_y_yagi/rails/railties/lib/rails/command.rb:34:in `invoke'
	from /home/yaginuma/program/rails/master_y_yagi/rails/railties/lib/rails/commands.rb:16:in `<top (required)>'
	from ./bin/rails:9:in `require'

The ServerCommand inherits Thor, but it does not use Thor is the
parse of the argument.
Therefore, if leave the argument of Thor as it is, it becomes an error by the
argument checking of Thor.
To avoid it, to clear the argument of Thor.

Fixes #26964

rails-bot · 2016-11-05T23:38:23Z

r? @pixeltrix

(@rails-bot has picked a reviewer for you, use r? to override)

y-yagi · 2016-11-05T23:38:49Z

r? @kaspth

rafaelfranca · 2016-11-06T00:05:11Z

Can we use the Thor option parser instead of reimplementing it?

y-yagi · 2016-11-06T22:58:55Z

If I am not mistaken, it is possible. I will try.

arthurnn · 2016-11-07T16:57:43Z

r? @rafaelfranca

rafaelfranca · 2016-11-07T18:11:16Z

railties/lib/rails/commands/server/server_command.rb

-    def initialize(*)
-      super
+    def initialize(options = nil)
+      super(options)


Does not only super work here?

Probably not. See my comment about Rack::Server interop.

rafaelfranca · 2016-11-07T18:15:09Z

railties/lib/rails/commands/server/server_command.rb

+      DEFAULT_PID_PATH = File.expand_path("tmp/pids/server.pid").freeze
+
+      class_option :port, aliases: "-p", type: :numeric,
+        desc: "Runs Rails on the specified port.", banner: :port


Can we keep the default in the description?

rafaelfranca · 2016-11-07T18:15:26Z

railties/lib/rails/commands/server/server_command.rb

+      class_option :port, aliases: "-p", type: :numeric,
+        desc: "Runs Rails on the specified port.", banner: :port
+      class_option :binding, aliases: "-b", type: :string,
+        desc: "Binds Rails to the specified IP.", banner: :IP


Same thing about the default

rafaelfranca · 2016-11-07T18:29:52Z

railties/test/commands/server_test.rb

+  private
+
+    def parse_arguments(args)
+      Rails::Command::ServerCommand.class_eval do


I believe you can asset those things only initializing a command object. Rails::Command::ServerCommand.new should expose everything you can.

Not sure, I had to do something similar for the other commands. But if we can make it happen, great 👍

kaspth · 2016-11-07T19:10:35Z

railties/lib/rails/commands/server/server_command.rb

+        end
+
+        def environment
+          options[:environment] || (ENV["RAILS_ENV"] || ENV["RACK_ENV"] || "development")


I wrote a Rails::Command.environment method for this.

kaspth · 2016-11-07T19:11:56Z

railties/lib/rails/commands/server/server_command.rb

+        end
+
+        def restart_command
+          "bin/rails server #{ARGV.join(' ')}"


I don't think the command infrastructure guarantees argv to be unharmed. We should add one test that uses every option and ensures every one option is present in the rerun command.

kaspth · 2016-11-07T19:12:17Z

railties/test/commands/server_test.rb

  ensure
    ARGV.replace original_args
  end
+
+  private
+


kaspth · 2016-11-07T19:13:06Z

railties/test/commands/server_test.rb

+  private
+
+    def parse_arguments(args)
+      Rails::Command::ServerCommand.class_eval do


Not sure, I had to do something similar for the other commands. But if we can make it happen, great 👍

kaspth · 2016-11-07T19:15:08Z

railties/lib/rails/commands/server/server_command.rb

+      class_option :daemon, aliases: "-d", type: :boolean, default: false,
+        desc: "Runs server as a Daemon."
+      class_option :environment, aliases: "-e", type: :string,
+        desc: "Specifies the environment to run this server under (test/development/production).", banner: :name


(development/test/production) is the usual order we'd write this in.

kaspth · 2016-11-07T19:17:31Z

railties/lib/rails/commands/server/server_command.rb

@@ -89,18 +41,6 @@ def middleware
      Hash.new([])
    end

-    def default_options


The reason I didn't pursue writing these options using Thor in the first place was that Rails::Server interops with Rack::Server in certain ways as seen by the super here.

Thus, since some of the methods you removed were also public, I figured they were part of Rails::Server's public API and to remove them we'd need to go through a deprecation cycle.

cc @rafaelfranca

Thank you for pointing out!
I fixed that Rails::Server's public API can be used as before. How about that?

kaspth · 2016-11-07T19:18:11Z

railties/lib/rails/commands/server/server_command.rb

-    def initialize(*)
-      super
+    def initialize(options = nil)
+      super(options)


Probably not. See my comment about Rack::Server interop.

kaspth · 2016-11-07T19:18:34Z

railties/lib/rails/commands/server/server_command.rb

-        options
-      end
-
-      def option_parser(options) # :nodoc:


This is also a method that's overriden from Rack::Server.

@kasper

@kasper's the best person to review this.

guilleiguaran · 2016-11-18T16:06:46Z

./bin/rails runner is failing for me using this branch when passing a second arg (the arg for the command that I'm trying to run with runner)

guilleiguaran · 2016-11-18T16:09:15Z

My bad, looks like this PR is fixing only the rails server command not the rails runner one

kaspth · 2016-11-18T16:09:31Z

@guilleiguaran correct. I'll handle the runner command not working with multiple args like it used to.

y-yagi · 2016-12-06T22:57:24Z

I was misunderstanding. Option parser is necessary even in the rack server. Therefore, I restored Rails::Server::Options.

kaspth

Think we're getting close ✌️

kaspth · 2016-12-22T20:11:11Z

railties/lib/rails/commands/server/server_command.rb

+        end
+
+        def restart_command
+          "bin/rails server #{ARGV.join(' ')}"


We need to be careful when mentioning ARGV in commands. I think this should be:

"bin/rails server #{@server} #{args.join(" ")}"

(Note that it needs to use double quoted strings here too.)

In this context, the args variable does not contain any arguments.
Therefore, kept the argument in the constructor and used it.

kaspth · 2016-12-22T20:12:26Z

railties/lib/rails/commands/server/server_command.rb

          # Require application after server sets environment to propagate
          # the --environment option.
          require APP_PATH
          Dir.chdir(Rails.application.root)
          server.start
        end
      end
+
+      def server_options


In Thor any public instance method is a command, so we'd need to wrap this in a no_commands do block.

kaspth · 2016-12-22T20:13:36Z

railties/lib/rails/commands/server/server_command.rb

-    def initialize(*)
-      super
+    def initialize(options = nil)
+      @default_options = options.nil? ? {} : options


How about this? @default_options = options || {}

kaspth · 2016-12-22T20:20:24Z

railties/lib/rails/commands/server/server_command.rb

-
-      def option_parser(options) # :nodoc:
-        OptionParser.new do |opts|
-          opts.banner = "Usage: rails server [puma, thin etc] [options]"


Can't we port over the server argument with this:

argument :server, optional: true, banner: '[puma, thin etc]'

If argument used, need to specify the "server" argument twice to specify server(e.g. ./bin/rails server server webrick).
Therefore, override .banner instead.

The `ServerCommand` inherits Thor, but currently does not use Thor option parser. Therefore, if leave the argument of Thor as it is, it becomes an error by the argument checking of Thor. To avoid it, to use the Thor option parser instead of reimplementing it. Fixes rails#26964

y-yagi · 2016-12-24T03:28:21Z

Thanks for reviewing! I updated commit!

kaspth · 2016-12-29T19:10:09Z

Way cool, thanks!

y-yagi · 2016-12-30T11:29:51Z

Thanks!!

DougMaisells · 2017-11-21T19:25:36Z

I am running Rails 5.1.1 and I get an error that looks like the same thing. What am I missing?
rails s –p 5000
/Users/douglas.maisells/.rvm/gems/ruby-2.3.1@chronus/gems/thor-0.19.4/lib/thor/base.rb:484:in handle_argument_error': ERROR: "rails server" was called with arguments ["5000"] (Thor::InvocationError) Usage: "rails server [puma, thin etc] [options]" from /Users/douglas.maisells/.rvm/gems/ruby-2.3.1@chronus/gems/thor-0.19.4/lib/thor/command.rb:34:in rescue in run'

To fix this, I worked another dev and we updated the Gemfile.lock and I changed, under railties, thor (~> 0.20.0), then I ran #bundle update thor , then I was able to execute the rails command above.

`optparse` is unused since rails#26977.

rails-bot assigned pixeltrix Nov 5, 2016

rails-bot assigned kaspth and unassigned pixeltrix Nov 5, 2016

maclover7 added the railties label Nov 6, 2016

y-yagi force-pushed the fix_26964 branch 2 times, most recently from 794b098 to 4597b09 Compare November 7, 2016 13:20

rails-bot assigned rafaelfranca and unassigned kaspth Nov 7, 2016

rafaelfranca previously requested changes Nov 7, 2016

View reviewed changes

kaspth reviewed Nov 7, 2016

View reviewed changes

y-yagi assigned kaspth and unassigned rafaelfranca Nov 7, 2016

y-yagi force-pushed the fix_26964 branch from 4597b09 to 856c1e8 Compare November 13, 2016 23:20

y-yagi changed the title ~~clear the args of Thor when start rails server~~ use Thor option parser in server commands parse Nov 13, 2016

kaspth mentioned this pull request Nov 23, 2016

rails server rejects options #27155

Closed

georgeclaghorn mentioned this pull request Dec 3, 2016

Rails 5.1.0.alpha can not start server with options #27260

Closed

y-yagi force-pushed the fix_26964 branch from 856c1e8 to 004e415 Compare December 6, 2016 09:34

kaspth reviewed Dec 22, 2016

View reviewed changes

y-yagi force-pushed the fix_26964 branch from 004e415 to 6547042 Compare December 24, 2016 03:21

Merge branch 'master' into fix_26964

ca3eb2c

kaspth merged commit e983948 into rails:master Dec 29, 2016

y-yagi deleted the fix_26964 branch December 30, 2016 11:29

tgxworld pushed a commit to tgxworld/rails that referenced this pull request Jun 18, 2018

Remove unused require

a865f62

`optparse` is unused since rails#26977.

use Thor option parser in server commands parse #26977

use Thor option parser in server commands parse #26977

Conversation

y-yagi commented Nov 5, 2016

Summary

rails-bot commented Nov 5, 2016

y-yagi commented Nov 5, 2016

rafaelfranca commented Nov 6, 2016

y-yagi commented Nov 6, 2016

arthurnn commented Nov 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilleiguaran commented Nov 18, 2016

guilleiguaran commented Nov 18, 2016

kaspth commented Nov 18, 2016

y-yagi commented Dec 6, 2016

kaspth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y-yagi commented Dec 24, 2016

kaspth commented Dec 29, 2016

y-yagi commented Dec 30, 2016

DougMaisells commented Nov 21, 2017 • edited

DougMaisells commented Nov 21, 2017 •

edited