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

Log a warning when more than one IoC dependency is found in the classpath #594

Merged
merged 3 commits into from
Oct 6, 2013
Merged

Log a warning when more than one IoC dependency is found in the classpath #594

merged 3 commits into from
Oct 6, 2013

Conversation

arikogan
Copy link

It has happened to us that people put more than one IoC dependency into their classpath (e.g. cucumber-spring AND cucumber-guice) and suddenly their beans are not being wired correctly. This is because when Cucumber detects such situation it silently defaults to DefaultJavaObjectFactory which doesn't provide IoC features.

This pull request adds a warning so at least this situation can be spotted while reading the log.

@@ -48,6 +48,10 @@
<artifactId>cobertura</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove slf4j and replace the logging with a simple System.err.println.

@arikogan
Copy link
Author

Well spotted, thanks for that. Please let me know if you are ok with the changes.

@aslakhellesoy
Copy link
Contributor

Thanks, that works, but I think it would be even better if you added two exception classes - NoInstances and TooManyInstances. And then a catch block for each one. It's a little ghetto to look at the exception's message to separate the two cases ;-)

@arikogan
Copy link
Author

I absolutely agree with you. I only did it because, based on what I've seen, there is not a hierarchy under CucumberException, only two classes extend it. Happy do it though, it's much more elegant.

@arikogan
Copy link
Author

Hi Aslak. Have you had the chance to look at this? Thanks.

@aslakhellesoy
Copy link
Contributor

Hi Ariel - not yet, but it looks good. @ilanpillemer could you merge this in please?

@brasmusson brasmusson merged commit e1b50ca into cucumber:master Oct 6, 2013
@brasmusson
Copy link
Contributor

Merged. Thanks for your effort @arikogan.

@brasmusson
Copy link
Contributor

@aslakhellesoy @ilanpillemer I took me the liberty to merge this PR.

@aslakhellesoy
Copy link
Contributor

Thanks @brasmusson

@ilanpillemer still want to be part of the team?

@arikogan
Copy link
Author

arikogan commented Oct 6, 2013

Thanks very much @brasmusson and @aslakhellesoy.

@lock
Copy link

lock bot commented Oct 25, 2018

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.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants