print spring client pid on command run #455

sonalkr132 · 2015-12-12T18:32:14Z

Resolves #407

rails-bot · 2015-12-12T18:32:15Z

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

grosser · 2015-12-12T22:52:30Z

how about:

when started via spring rails xxx don't print
when started via rails xxx print by default ?

sonalkr132 · 2015-12-13T05:11:39Z

@grosser I don't know if I got this right, but for this we need to differentiate between two types of commands (namely spring rails/rake xxx and rails/rake xxx).
This line is the first thing that gets called:

Spring::Client.run(ARGV)

ARGV is same for both cases (spring rails c and rails c). Probably cause bin/rails acts a stand-in and just delegates commands to bin/spring leaving no difference between two commands.
Can you think of a way to differentiate between two commands?

grosser · 2015-12-13T05:15:43Z

$0 for bin/rails is ./bin/rails and for spring is rails_runner

On Sat, Dec 12, 2015 at 9:11 PM, Aditya Prakash [email protected]
wrote:

@grosser https://github.com/grosser I don't know if I got this right,
but for this we need to differentiate between two types of commands (namely spring
rails/rake xxx and rails/rake xxx).
This https://github.com/rails/spring/blob/master/bin/spring#L49 line is
probably the first line that gets called:

Spring::Client.run(ARGV)

ARGV is same for both cases (spring rails c and rails c). Probably cause
bin/rails acts a stand-in and just delegates commands to bin/spring
leaving no difference between two commands.
Can you think of a way to differentiate between two commands?

—
Reply to this email directly or view it on GitHub
#455 (comment).

sonalkr132 · 2015-12-13T06:35:13Z

done!

grosser · 2015-12-13T06:50:49Z

bin/spring

@@ -46,4 +46,5 @@ end
 lib = File.expand_path("../../lib", __FILE__)
 $LOAD_PATH.unshift lib unless $LOAD_PATH.include?(lib) # enable local development
 require 'spring/client'
+Spring.verbose_output = false if $0.include? 'spring' # don't output running spring status if command has `spring` in it


how about: Spring.verbose_output = $0.include?('spring')

the comment repeats the code, how about:

if the user knows that spring is called, do not show that spring is called

how about: Spring.verbose_output = $0.include?('spring')

I think you meant Spring.verbose_output = !$0.include?('spring')
It is not going to work cause it will set Spring.verbose_output to true when command includesspring, even when user has Spring.verbose_output = false defined in config/spring.rb (or ~/.spring.rb).

ah yeah, makes sense

On Sat, Dec 12, 2015 at 11:44 PM, Aditya Prakash [email protected]
wrote:

In bin/spring
#455 (comment):

@@ -46,4 +46,5 @@ end
lib = File.expand_path("../../lib", FILE)
$LOAD_PATH.unshift lib unless $LOAD_PATH.include?(lib) # enable local development
require 'spring/client'
+Spring.verbose_output = false if $0.include? 'spring' # don't output running spring status if command has spring in it

how about: Spring.verbose_output = $0.include?('spring')

I think you meant Spring.verbose_output = !$0.include?('spring')
It is not going to work it will set Spring.verbose_output to true when
command includesspring, even when user has Spring.verbose_output = false
defined in config/spring.rb (or ~/.spring.rb).

—
Reply to this email directly or view it on GitHub
https://github.com/rails/spring/pull/455/files#r47441438.

grosser · 2015-12-13T06:52:29Z

👍 @jonleighton looks good ?

jonleighton · 2015-12-14T13:55:25Z

lib/spring/configuration.rb

@@ -49,4 +49,6 @@ def find_project_root(current_dir)
      end
    end
  end
+
+  self.verbose_output = true


Calling the option "verbose" makes me think that it produces more than the normal amount of output. However the default is "verbose" mode.

I'd call this Spring.quiet instead, and have it default to false. Then you're opting in to quietness, rather than the opting out of verbosity. (I also think the _output bit is superfluous.)

jonleighton · 2015-12-14T14:04:02Z

Made some comments but generally 👍

sonalkr132 · 2015-12-14T15:25:07Z

@jonleighton @grosser Thanks for getting me through it 😝

I have made the changes you requested.

grosser · 2015-12-14T16:39:59Z

lib/spring/test/acceptance_test.rb

@@ -94,6 +102,22 @@ def without_gem(name)
        refute app.spring_env.server_running?
      end

+      test "running spring status for rails/rake commands" do


these descriptions could make it easier for other users to understand what is being tested:

test "tells the user that spring is being used when used automatically via binstubs" test "does not tell the user that spring is being used when the user used spring manually" test "does not tell the user that spring is being when used automatically via binstubs but quiet is enabled"

grosser · 2015-12-14T17:28:26Z

thx, I'll merge when travis is green :)

sonalkr132 · 2015-12-14T17:41:16Z

hey, a quick question: Is watch_interval config serving any purpose? It is mentioned in lib/spring/watcher.rb but never really used any where else in repo. Check this

grosser · 2015-12-14T17:44:51Z

looks unused ... can you make a cleanup PR to remove it ?

On Mon, Dec 14, 2015 at 9:41 AM, Aditya Prakash [email protected]
wrote:

hey, a quick question: Is watch_interval config serving any purpose? It
is mentioned in lib/spring/watcher.rb but never really used any where
else in repo. Check this
https://github.com/rails/spring/search?utf8=%E2%9C%93&q=watch_interval

—
Reply to this email directly or view it on GitHub
#455 (comment).

grosser · 2015-12-14T17:45:10Z

here we got!

print spring client pid on command run

grosser · 2015-12-14T17:48:12Z

have fun with 1.6.0 :)

sonalkr132 · 2015-12-14T17:50:12Z

yay 🎉

jonleighton · 2015-12-15T15:42:42Z

hey, a quick question: Is watch_interval config serving any purpose? It is mentioned in lib/spring/watcher.rb but never really used any where else in repo. Check this

I think I just put it there to avoid having magic numbers in the code. I don't think we should hardcode something like this.

sonalkr132 · 2015-12-15T16:11:02Z

Is watch_interval config serving any purpose? It is mentioned in lib/spring/watcher.rb but never really used any where else in repo

looks unused ... can you make a cleanup PR to remove it?

I was wrong that it is not serving any purpose. (check this travis run).
@jonleighton is it defined in some library spring is using? Which class behavior does it affect?

hsume2 · 2015-12-16T22:00:07Z

lib/spring/client/run.rb

@@ -1,3 +1,4 @@
+require "spring/commands"


@sonalkr132 hey, quick question. I was wondering why you added this line? I'm getting an exception which seems to go away when I remove this line (which I've logged here).

I had to require spring/command cause it loads the user defined config files (config/spring.rb or ~/.spring.rb).

If user has defined Spring.quite on one of those files then we need different behavior here:

puts "Running via Spring preloader in process #{pid}" unless Spring.quiet

Got it, thanks!

@sonalkr132 @grosser I missed this, but we shouldn't be requiring spring/commands in the client. apart from this issue that @hsume2 reported, loading bundler in the client slows down the execution of spring commands, which is exactly what we want to avoid. The client needs to run as quickly as possible.

For client-side configuration, there is the config/spring_client.rb file, so in theory the user could set Spring.quiet in there (although I don't think this is really something we need to document...)

After fixing this, it would be great if we could somehow prevent a similar bug from being reintroduced in the future (perhaps we can somehow write a test which asserts that bundler is not present in the client? I don't know)

So,Spring.quite option can only be set in spring_client.rb?

With the current implementation, I think the answer should be yes. However I think that actually we could write this "Running via Spring preloader" message in Application#serve, by which point spring/commands would be loaded. So I think that may be a better option.

rails-bot assigned rafaelfranca Dec 12, 2015

grosser reviewed Dec 13, 2015
View reviewed changes

sonalkr132 force-pushed the notif-running branch 6 times, most recently from 652d544 to e06c4fd Compare December 13, 2015 08:27

jonleighton reviewed Dec 14, 2015
View reviewed changes

sonalkr132 force-pushed the notif-running branch from e06c4fd to 70207b9 Compare December 14, 2015 15:13

print spring client pid on command run

4dd5e03

sonalkr132 force-pushed the notif-running branch from 70207b9 to 8964a36 Compare December 14, 2015 15:15

grosser reviewed Dec 14, 2015
View reviewed changes

sonalkr132 force-pushed the notif-running branch from 8964a36 to 3fba9df Compare December 14, 2015 17:13

sonalkr132 added 2 commits December 14, 2015 22:52

test Spring.quite config

347731e

Do not output running spring status for commands with prefix 'spring'

20383d6

sonalkr132 force-pushed the notif-running branch from 3fba9df to 20383d6 Compare December 14, 2015 17:24

grosser added a commit that referenced this pull request Dec 14, 2015

Merge pull request #455 from sonalkr132/notif-running

47a4e71

print spring client pid on command run

grosser merged commit 47a4e71 into rails:master Dec 14, 2015

hsume2 reviewed Dec 16, 2015
View reviewed changes

sonalkr132 mentioned this pull request Dec 28, 2015

require "spring/commands" after boot_server #460

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

print spring client pid on command run #455

print spring client pid on command run #455

sonalkr132 commented Dec 12, 2015

rails-bot commented Dec 12, 2015

grosser commented Dec 12, 2015

sonalkr132 commented Dec 13, 2015

grosser commented Dec 13, 2015

sonalkr132 commented Dec 13, 2015

grosser Dec 13, 2015

grosser Dec 13, 2015

sonalkr132 Dec 13, 2015

grosser Dec 13, 2015

grosser commented Dec 13, 2015

jonleighton Dec 14, 2015

jonleighton commented Dec 14, 2015

sonalkr132 commented Dec 14, 2015

grosser Dec 14, 2015

sonalkr132 Dec 14, 2015

grosser commented Dec 14, 2015

sonalkr132 commented Dec 14, 2015

grosser commented Dec 14, 2015

grosser commented Dec 14, 2015

grosser commented Dec 14, 2015

sonalkr132 commented Dec 14, 2015

jonleighton commented Dec 15, 2015

sonalkr132 commented Dec 15, 2015

hsume2 Dec 16, 2015

sonalkr132 Dec 17, 2015

hsume2 Dec 17, 2015

jonleighton Dec 24, 2015

sonalkr132 Dec 24, 2015

jonleighton Dec 24, 2015

print spring client pid on command run #455

print spring client pid on command run #455

Conversation

sonalkr132 commented Dec 12, 2015

rails-bot commented Dec 12, 2015

grosser commented Dec 12, 2015

sonalkr132 commented Dec 13, 2015

grosser commented Dec 13, 2015

sonalkr132 commented Dec 13, 2015

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

grosser commented Dec 13, 2015

Choose a reason for hiding this comment

jonleighton commented Dec 14, 2015

sonalkr132 commented Dec 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grosser commented Dec 14, 2015

sonalkr132 commented Dec 14, 2015

grosser commented Dec 14, 2015

grosser commented Dec 14, 2015

grosser commented Dec 14, 2015

sonalkr132 commented Dec 14, 2015

jonleighton commented Dec 15, 2015

sonalkr132 commented Dec 15, 2015

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