Don't supress non-spring load errors in binstub

[felixbuenemann]

Without this change, the spring binstub supresses unrelated load errors, eg. due to errors in initializers or loaded gems. This change causes the binstub to only ignore a missing spring loader or gem.

This will get rid of the misleading warning: already initialized constant APP_PATH and resolves #259.

[rails-bot]

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.

[felixbuenemann]

The acceptance tests currently only cover the case of a missing spring gem, but not of a missing spring binary. If a missing spring binary should blow up with a load error, I could omit the check for spring_loader.

[felixbuenemann]

Hmm, seems like Ruby 1.9.3 doesn't have LoadError#path, I could check the exception message instead.

[felixbuenemann]

OK, updated to use LoadError#message instead of #path.

[grosser]

can you add an acceptance test that fails without this patch ? (like loading a random gem and asserting it blows up nicely)

[grosser]

doing the assignment on an extra line would improve readability a bit

[felixbuenemann]

Yes, but it would also break the existing regex to check for the old/new style binstub.

[grosser]

how is this breaking a regex ?

spring_loader = File.expand_path('../spring', __FILE__)
load spring_loader

[felixbuenemann]

Good catch, both load File.expand_path('../spring', __FILE__) and load spring_loader match the regex /load .*spring/ in Spring::Binstub::Item#add, so it's OK.

[grosser]

I think that's a great change/pattern, too many libraries do this rescue all ...

[chancancode]

I guess this supersedes #370 😁 👏

Can you find out if there are any relevant comment from Jon and add "Closes #370" to your commit message?

Thanks for picking this up!

[felixbuenemann]

@chancancode Oh, I didn't see your PR because it wasn't linked to the issue mentioned above.

I think we could add a test to check for the case were the spring binary is missing.

@grosser What do you think should be the intended behavior, when the spring binary is missing? (Example: There's a springified bin/rails, but bin/spring has been deleted.) Should a LoadError be raised, or should it be ignored (current but maybe unintended behavior).

[grosser]

is there any downside to ignoring it ? wrong version will be used / slower ?

On Mon, Nov 16, 2015 at 6:42 AM, Felix Bünemann [email protected]
wrote:

@chancancode https://github.com/chancancode Oh, I didn't see your PR
because it wasn't linked to the issue mentioned above.

I think we could add a test to check for the case were the spring binary
is missing.

@grosser https://github.com/grosser What do you think should be the
intended behavior, when the spring binary is missing? (Example: There's a
springified bin/rails, but bin/spring has been deleted.) Should a LoadError
be raised, or should it be ignored (current but maybe unintended behavior).

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

[felixbuenemann]

Well, it should only happen, if a user deleted bin/spring without restoring the original binstubs. There are no performance problems and given that there doesn't seem to be a command to "unspringify" the binstubs we should probably handle that case.
My initial thought for not ignoring it, was to alert the user that their spring setup is broken. While the second case (missing gem) is expected, eg. because spring is only loaded in the development environment.

[grosser]

So sounds like you think it's best to ignore it ?

On Mon, Nov 16, 2015 at 8:41 AM, Felix Bünemann [email protected]
wrote:

Well, it should only happen, if a user deleted bin/spring without
restoring the original binstubs. There are no performance problems and
given that there doesn't seem to be a command to "unspringify" the binstubs
we should probably handle that case.
My initial thought for not ignoring it, was to alert the user that their
spring setup is broken. While the second case (missing gem) is expected,
eg. because spring is only loaded in the development environment.

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

[felixbuenemann]

Yes, I'll add a test that makes the expected behavior explicit.

[felixbuenemann]

Adressed feedback from @grosser and @chancancode and added a new test that checks the behavior in absence of a bin/spring binary.

[grosser]

🎉

[grosser]

1.4.2 is out !

[felixbuenemann]

Great, thanks Michael!