Allow rspec-rails to load without Rubygems loaded

[wanelo-pair]

Rspec-rails is a great gem citizen, and doesn't explicitly require Rubygems anywhere. However, while working on a Rails app that doesn't require Rubygems before loading Rails, we've noticed that rspec-rails expects Rubygems to be loaded even though it doesn't require it.

This pull request simply checks for Rubygems before calling Rubygems methods, so that rspec-rails works without Rubygems already being loaded.

[indirect]

👍

[fables-tales]

this is LGTM. @cupakromer @myronmarston got thoughts?

[myronmarston]

This LGTM, although I wonder if we can take it a step further and just use require 'minitest' with no gem statement at all...why is that even needed?

[indirect]

Minitest itself tells you to "gem 'minitest'". AFAIK, you could just require it, though.

On Tue, Jun 17, 2014 at 10:33 AM, Myron Marston [email protected]
wrote:

This LGTM, although I wonder if we can take it a step further and just use require 'minitest' with no gem statement at all...why is that even needed?

Reply to this email directly or view it on GitHub:
#1099 (comment)

[myronmarston]

Minitest itself tells you to "gem 'minitest'". AFAIK, you could just require it, though.

Do we know what the risk of not doing gem "minitest" is?

[indirect]

Minitest might not be loaded. :) Here's the implementation of gem. It finds a gemspec, sets up the load path, etc. With Rubygems loaded, require is intercepted to do the same thing. With Rubygems not loaded, the call to gem is totally unneeded.

[cupakromer]

If we can I'd prefer to drop gem if we can. Now that Minitest is going to be out of core, but included as a gem I wonder if this will make a difference. Let me check what rails does.

[cupakromer]

Right now I found: https://github.com/rails/rails/blob/e20dd73df42d63b206d221e2258cc6dc7b1e6068/activesupport/lib/active_support/testing/autorun.rb

Though that doesn't appear to be required in any lib path. Seems it's manually added to test_helper. The current rails master branch puts minitest in it's gemspec, so it will get setup that way and it seems they just use normal require.

We'd need to check the different rails 3.x versions too :-/

[indirect]

I have an open pull request with rails to stop calling gem if it's not defined as well. I'm happy with just a conditional, but I'm also pretty sure that dropping them gem call will work just fine.

On Tue, Jun 17, 2014 at 12:01 PM, Aaron Kromer [email protected]
wrote:

Right now I found: https://github.com/rails/rails/blob/e20dd73df42d63b206d221e2258cc6dc7b1e6068/activesupport/lib/active_support/testing/autorun.rb
Though that doesn't appear to be required in any lib path. Seems it's manually added to test_helper. The current rails master branch puts minitest in it's gemspec, so it will get setup that way and it seems they just use normal require.

We'd need to check the different rails 3.x versions too :-/

Reply to this email directly or view it on GitHub:
#1099 (comment)

[myronmarston]

Thinking about this some more...it'd be nice to drop the gem 'minitest' entirely but there's risk of breakage for users, and I don't think the benefit of dropping it is enough to warrant dropping it in a 3.x release. Maybe in 4.0?

[indirect]

I’m perfectly happy with not dropping it, I’m just hoping to not call it if rubygems isn’t loaded. :)

On Jun 17, 2014, at 12:36 PM, Myron Marston [email protected] wrote:

Thinking about this some more...it'd be nice to drop the gem 'minitest' entirely but there's risk of breakage for users, and I don't think the benefit of dropping it is enough to warrant dropping it in a 3.x release. Maybe in 4.0?

—
Reply to this email directly or view it on GitHub.

[wanelo-pair]

Is there anything I can change that would make this pull more acceptable? Thanks!

[myronmarston]

It looks fine to me as-is. One part of the travis build failed with a transient error, so I restarted it. I'll leave it to @cupakromer to merge it.

[cupakromer]

Yeah, I'm good with this. I plan on leaving this on the 3.1.x line only as a new feature. Let me know if anyone has any objections to that.

[myronmarston]

Yeah, I'm good with this. I plan on leaving this on the 3.1.x line only as a new feature. Let me know if anyone has any objections to that.

It's your call on this, but my instinct is that this is a bug fix. Without this change, don't users get a NoMethodError "gem" if rubygems is not loaded? I generally try to include all bug fixes in the next patch release unless there's some reason not to.

[JonRowe]

I'd say this was a bug fix too.

[cupakromer]

K. Done and done. (ᵔᴥᵔ)