-
-
Notifications
You must be signed in to change notification settings - Fork 327
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
Do not require RSpec if not already loaded #448
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This helps people use the parts that they need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whilst the file already has conditionals. Given these are procedural loaders I'd rather remove conditionals.
Maybe look into making another file for minitest?
Actually having re-reviewed the codebase. If you want to add a 1liner to include the extra file. I'm happy with that as a bug request fix. But altering the requires would be a feature change (which is fine), but I'd want that tweaking in other files ideally. The reason being is that the one autoload file which "isn't" autoloaded is this file (For a reason I'm not sure of). Essentially a user could in theory use cucumber without either of rspec or minitest in place (Which I'm not sure is great) For your specific use case (Both loaded), this probably needs some form of manual user input. So a user would load something like Also there's some argument it might be a major change (Again not a problem), but would need treating accordingly. |
We could do something to the inverse maybe to avoid the breaking change? Maybe something like: if minitest is loaded and RSpec is not, then dont require RSpec? |
Sounds like a good idea. So basically we have 4 situations.
Atm we load RSpec when only RSpec is present (:green_book:) Anything you can do RE that is good in my eyes. If you wanted to check if these were already pre-loaded we could. However I don't know what users allow delegate their loading to us. i.e. it could be people just require 'cucumber/rails/rspec' I know I've probably opened up a can of worms here. So I'll apologise for that. Even the name of the file gives away we perhaps are a bit behind the times here. |
@luke-hill Would that translate to: change the |
Possibly. But I think a better approach would be to have a minitest initializer and an rspec one. Because at the moment, the rspec one (named as such), is controlling minitest because we (perhaps incorrectly), assumed that if one existed, the other didn't. And one always existed. In the truth table, there's 4 situations. We need to cover each of them ideally. The OP did suggest we could cover 2 of the use cases (1 is loaded and the other isn't), by just checking for each of them |
@jsmestad any thoughts on this? |
@luke-hill I think the request became larger than what I was expecting and had to move onto other tasks. Since then we just switched to RSpec completely to avoid the issue altogether. I agree with the approach, but the time involved was going to be too much for me to take on during year-end scramble. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good to me, rspec should not be a forced dependencies
I'll kill the PR @jsmestad but leave the issue open so someone else can come into it. I'm thinking this will be hard to fix in a regular PR, but we could make a brute force change and cut a 3.0 release and just say "cucumber rails always had an assumption you would use rspec, but now you need to explicitly tell it you want to use rspec, or minitest" |
Summary
Fixes #446
How Has This Been Tested?
Types of changes
Checklist: