-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Ensure rspec core is loaded if rspec-rails is. #1558
Conversation
@rspec/rspec I'm surprised we didn't do this already. Anyone know any relevant history? |
I think we've never done this because it's generally not needed. The standard way to run RSpec is via rspec-core's Having a require here doesn't cause any problems, of course, but I don't understand why it's necessary, either. |
@myronmarston see linked fivemat issue for justification |
I saw that, but I didn't quite understand yet. Here's what I understand so far:
I get that, but what I don't understand is....how is I think whether or not we should do this depends on how the user is running RSpec and if its a way that we want to support. |
There nothing in the linked issue to justify this, |
Fivemat is loaded in that because of |
I kind of feel fivemat should be solving this, maybe they should require it before checking? |
Turns out this is a spring issue... |
I don't think loading rspec/rails here is necessarily right, since Rails might not be required yet and I don't think we should be the ones to force that. Could be wrong here, but only loading rspec is more conservative so choosing that for now. Loading rspec seems important though, so that various version checks work correctly. See tpope/fivemat#29 for motivation.
@rspec/rspec now this is green, are there any objections to merging this? It seems totally harmless to me, even if the justification is a little weird. |
If this actually fixes the issue in question, we can merge it (although I'd want a comment one the line explaining why, since it's an odd line that shouldn't needed). If it does not fix the issue in question, I'd prefer we not merge it. I really dislike having lines of code that are "just in case" because there's a chance it might fix an issue for someone, but we're really not sure, and there's no test documenting the case it fixes. |
I think we might want to consider reverting this. I believe it is the change that introduced #1645 and also changed the behavior of having Given that, should we revert this? |
You have my 👍 for a revert. |
yeah revert it On Fri, Jul 22, 2016, at 04:22 PM, Myron Marston wrote:
Links:
|
I don't think loading rspec/rails here is necessarily right, since Rails
might not be required yet and I don't think we should be the ones to
force that. Could be wrong here, but only loading rspec is more
conservative so choosing that for now.
Loading rspec seems important though, so that various version checks
work correctly.
See tpope/fivemat#29 for motivation.