Skip to content
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

Generic driver #99

Merged
merged 6 commits into from
Mar 13, 2016
Merged

Generic driver #99

merged 6 commits into from
Mar 13, 2016

Conversation

konserw
Copy link
Contributor

@konserw konserw commented Feb 13, 2016

My proposition for issue:
Error when no testing framework is detected #80

#ifdef CUKE_ENABLE_GENERICDRIVER
#include "GenericDriver.hpp"
#else
#error Please #inlude testing framework before cucumber-cpp or #define CUKE_ENABLE_GENERICDRIVER explicitly
Copy link
Member

Choose a reason for hiding this comment

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

Typo #inlude => #include

@paoloambrosio
Copy link
Member

PR looks OK (see comments) but I have to say that I don't like much the macro CUKE_ENABLE_GENERICDRIVER. What do you think of triggering behaviour based on a different include? It shouldn't require many changes.

Current

With testing framework:

#include <CppSpec/CppSpec.h>
#include <cucumber-cpp/defs.hpp>

Without testing framework:

#include <cucumber-cpp/defs.hpp>

Proposed

With testing framework (it errors if it can't find it):

#include <CppSpec/CppSpec.h>
#include <cucumber-cpp/autodetect.hpp>

Without testing framework (using the generic driver):

#include <cucumber-cpp/generic.hpp>

Under the hood, autodetect.hpp would include DriverSelector.hpp and that should error if it can't find it, instead generic.hpp would include GenericDriver.hpp.

@paoloambrosio
Copy link
Member

...or perhaps instead of introducing #include <cucumber-cpp/generic.hpp> we could keep using #include <cucumber-cpp/defs.hpp> if someone does not want the safety net of autodetect.hpp?

Revised proposal

With testing framework (it errors if it can't find it):

#include <CppSpec/CppSpec.h>
#include <cucumber-cpp/autodetect.hpp>

Without testing framework (using the generic driver as fallback if not found - backward compatible):

#include <cucumber-cpp/defs.hpp>

@paoloambrosio
Copy link
Member

Added a test for the Generic Driver and removed duplication in paoloambrosio/cucumber-cpp@22c427a. Please include it in this PR.
Also had a change of heart and thought the autoswitching between generic and not was too dangerous, so I've marked it as deprecated in paoloambrosio/cucumber-cpp@cae0f25. For this I'll let you decide what you prefer.

@konserw
Copy link
Contributor Author

konserw commented Mar 13, 2016

I think that both your commits are good ;) I'm testing them right now and I'll push them in a second :)

@paoloambrosio
Copy link
Member

In cases like 059878d + bd7871a, feel free to remove commits and use "push -f" to revert your own branch/PR so that the history reads better.

Is this ready to merge?

@konserw
Copy link
Contributor Author

konserw commented Mar 13, 2016

I've rebased it once again. I believe now it is ready ;)

@paoloambrosio paoloambrosio merged commit f37b951 into cucumber:master Mar 13, 2016
paoloambrosio added a commit that referenced this pull request Mar 13, 2016
@konserw konserw deleted the GenericDriver branch September 28, 2016 07:52
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.

2 participants