-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Guice module - add new scenario scope #683
Conversation
I posted a message to the list. Let's see if someone has any comments. If I don't hear anything for a week we can merge this in. |
I just returned from a couple of weeks holiday. Is this now going to be merged? |
This is already merged and part of the 1.1.6 release: https://github.com/cucumber/cucumber-jvm/blob/master/History.md Just forgot to close this. |
Could you please check again? I just pulled the latest code and I don't see the changes. Github is also telling me there are unmerged commits. |
@JakeCollins I'd say you are right, this PR is not merged. Only the attribution in History.md got into 1.1.6 (the attribution commit follows directly after the merge of another PR). |
@JakeCollins, @brasmusson is right. I intended to merge this, but I realise I never did. Can you rebase your branch with master so we can give it another try? Thanks, |
… the Guice scopes available for use in your test code.
@aslakhellesoy it's now rebased, please try again? |
@aslakhellesoy @brasmusson I guess you guys are busy, but just to remind you the |
@aslakhellesoy @brasmusson could you please give an update on this? Eg, will it be merged at some point? If not, no problem, but please let me know, then I can consider releasing it as a stand alone project. |
I can push the merge button. I'm going to go on the previous comments that nobody objected to it, and the rebase shows that it's good, and the travis build passed, so given the history I'm going to merge it. As well, it's two commits if we need to unmerge it :) |
Guice module - add new scenario scope
@dkowis please update History.md |
@aslakhellesoy yep, I'm typing it up right now :) |
@aslakhellesoy also, this is non-backwards compatible with the guice module, should it be at least 1.2? I don't remember if we'd update to 2.0 for this since it doesn't break the core API itself. |
@dkowis great news, thank you. When you decide what the release version number should be, could you please put it on line 13 and 16 of this file: (The file currently shows 1.1.6 which is now incorrect) |
Please remove version numbers in all files. We'll never remember to update them in the future. |
Ah never mind. I see it doesn't need to be updated in the future. |
@aslakhellesoy - Exactly, it's just telling people they need to migrate when they update to that version. So it still remains to decide, what should be the release version number? |
@JakeCollins It is actually not necessary to know the next release version number. "> 1.1.7" is correct regardless of the next version is 1.1.8 or 1.2.0. I have updated the number. |
@brasmusson good point! Thanks! |
@brasmusson lol! Yes, that was my thinking when I put '> 1.1.6' and 1.1.6 was the current released version. I forgot that when I made my earlier comment. I guess as a nicety, but no big deal, we could update line 16 only when the release version is known, eg '>= X.X.X ...' |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This commit adds a new Cucumber scenario scope to the Guice scopes available for use in your test code.
I realise this may be considered a controversial pull request as it's a complete rewrite of the module, rather than an incremental change. It's also not backwards compatible with the previous version of the module due to the new lifecycle of the Guice injector, see documentation below for details.
I'd be interested to hear whether the maintainers would consider rolling this out, and if so how it could fit into the roadmap given it's a breaking change?
From the new documentation in
package.html
: