Skip to content

Fix binstubs not being replaced when their quoting style was changed #534

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
Aug 10, 2017

Conversation

yhirano55
Copy link
Contributor

I found a bug: The same code is duplicatedly generated.

Background

I've run rubocop -a after bundle exec spring binstub --all.
And it changed to double quotes from single quotes in binstubs:

#!/usr/bin/env ruby
begin
  load File.expand_path("../spring", __FILE__)
rescue LoadError => e
  raise unless e.message.include?("spring")
end
APP_PATH = File.expand_path("../config/application", __dir__)
require_relative "../config/boot"
require "rails/commands"

Then I've run bundle exec spring binstub --all again.

Before

$ bundle exec spring binstub --all
* bin/rake: spring inserted # <- Oops
* bin/rails: spring inserted # <- Oops

$ git diff
diff --git a/bin/rails b/bin/rails
index 40d2d5e..7ee6e82 100755
--- a/bin/rails
+++ b/bin/rails
@@ -1,5 +1,10 @@
 #!/usr/bin/env ruby
 begin
+  load File.expand_path('../spring', __FILE__)
+rescue LoadError => e
+  raise unless e.message.include?('spring')
+end
+begin
   load File.expand_path("../spring", __FILE__)
 rescue LoadError => e
   raise unless e.message.include?("spring")
diff --git a/bin/rake b/bin/rake
index 3df5805..72f4b15 100755
--- a/bin/rake
+++ b/bin/rake
@@ -1,5 +1,10 @@
 #!/usr/bin/env ruby
 begin
+  load File.expand_path('../spring', __FILE__)
+rescue LoadError => e
+  raise unless e.message.include?('spring')
+end
+begin
   load File.expand_path("../spring", __FILE__)
 rescue LoadError => e
   raise unless e.message.include?("spring")
diff --git a/bin/spring b/bin/spring
index b44ad1a..fb2ec2e 100755
--- a/bin/spring
+++ b/bin/spring
@@ -4,14 +4,14 @@
 # It gets overwritten when you run the `spring binstub` command.

 unless defined?(Spring)
-  require "rubygems"
-  require "bundler"
+  require 'rubygems'
+  require 'bundler'

   lockfile = Bundler::LockfileParser.new(Bundler.default_lockfile.read)
   spring = lockfile.specs.detect { |spec| spec.name == "spring" }
   if spring
     Gem.use_paths Gem.dir, Bundler.bundle_path.to_s, *Gem.path
-    gem "spring", spring.version
-    require "spring/binstub"
+    gem 'spring', spring.version
+    require 'spring/binstub'
   end
 end

After

$ bundle exec spring binstub --all
* bin/rake: upgraded
* bin/rails: upgraded

$ git diff
diff --git a/bin/rails b/bin/rails
index 40d2d5e..8e69e09 100755
--- a/bin/rails
+++ b/bin/rails
@@ -1,8 +1,8 @@
 #!/usr/bin/env ruby
 begin
-  load File.expand_path("../spring", __FILE__)
+  load File.expand_path('../spring', __FILE__)
 rescue LoadError => e
-  raise unless e.message.include?("spring")
+  raise unless e.message.include?('spring')
 end
 APP_PATH = File.expand_path("../config/application", __dir__)
 require_relative "../config/boot"
diff --git a/bin/rake b/bin/rake
index 3df5805..53da8ce 100755
--- a/bin/rake
+++ b/bin/rake
@@ -1,8 +1,8 @@
 #!/usr/bin/env ruby
 begin
-  load File.expand_path("../spring", __FILE__)
+  load File.expand_path('../spring', __FILE__)
 rescue LoadError => e
-  raise unless e.message.include?("spring")
+  raise unless e.message.include?('spring')
 end
 require_relative "../config/boot"
 require "rake"
diff --git a/bin/spring b/bin/spring
index b44ad1a..fb2ec2e 100755
--- a/bin/spring
+++ b/bin/spring
@@ -4,14 +4,14 @@
 # It gets overwritten when you run the `spring binstub` command.

 unless defined?(Spring)
-  require "rubygems"
-  require "bundler"
+  require 'rubygems'
+  require 'bundler'

   lockfile = Bundler::LockfileParser.new(Bundler.default_lockfile.read)
   spring = lockfile.specs.detect { |spec| spec.name == "spring" }
   if spring
     Gem.use_paths Gem.dir, Bundler.bundle_path.to_s, *Gem.path
-    gem "spring", spring.version
-    require "spring/binstub"
+    gem 'spring', spring.version
+    require 'spring/binstub'
   end
 end

@rails-bot
Copy link

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @grosser (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.

@yhirano55 yhirano55 force-pushed the fix_bug_on_re_generating_binstub branch from 2306b04 to 260ced7 Compare August 8, 2017 14:57
Copy link
Collaborator

@grosser grosser left a comment

Choose a reason for hiding this comment

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

smells like a lot of copy-paste ... how about something like /#{Regexp.escape(LOADER).gsub('"', "['\"]")}/ or so ?

... also a test would be great to not break this in the next release

@yhirano55
Copy link
Contributor Author

@grosser I've fixed it and removed one line for refactoring. I haven't used Regexp.escape since ever. Thanks for the comment.

CHANGELOG.md Outdated
@@ -1,5 +1,6 @@
## Next release

* Fix a bug on re-generating binstub (#534)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fix binstubs not being replaced when their quoting style was changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This message describes perfectly this changes. Thanks so much. I'll change this.

@grosser
Copy link
Collaborator

grosser commented Aug 10, 2017

do any tests fail if you delete all the binstub code ?
if yes then can you copy/modify one of them to make this change tested ?

@yhirano55 yhirano55 changed the title Fix a bug on re-generating binstub Fix binstubs not being replaced when their quoting style was changed Aug 10, 2017
@yhirano55 yhirano55 force-pushed the fix_bug_on_re_generating_binstub branch from aff7afb to 51dec1a Compare August 10, 2017 06:07
@yhirano55
Copy link
Contributor Author

yhirano55 commented Aug 10, 2017

do any tests fail if you delete all the binstub code ?

No, tests didn't failed. But I've added a test case for re-create binstub.

@grosser grosser merged commit f0d2d65 into rails:master Aug 10, 2017
@yhirano55 yhirano55 deleted the fix_bug_on_re_generating_binstub branch August 10, 2017 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants