Skip to content

JavaHookDefinitions should never be static #2310

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

Closed
mpkorstanje opened this issue May 29, 2021 · 7 comments
Closed

JavaHookDefinitions should never be static #2310

mpkorstanje opened this issue May 29, 2021 · 7 comments
Labels
🐛 bug Defect / Bug good first issue Good for newcomers 🙏 help wanted Help wanted - not prioritized by core team

Comments

@mpkorstanje
Copy link
Contributor

mpkorstanje commented May 29, 2021

When using a JavaHookDefinition (@Before/After/BeforeStep/AfterStep`) Cucumber currently does not inform users that hook definitions probably should not be static. While the code itself is generic enough to correctly invoke a static hook definition it does seem to confuse novice users.

So createInvalidSignatureException should be invoked when a hook is static.

    @After
    public static void after() {
       // declaring this should result in an exception
    }
    

    @After
    public void after() {
       // this is fine
    }    
@mpkorstanje mpkorstanje added 🐛 bug Defect / Bug 🙏 help wanted Help wanted - not prioritized by core team good first issue Good for newcomers labels May 29, 2021
@laeubi
Copy link

laeubi commented May 29, 2021

I don't see why this should throw an exception just for the sake that spring-users are confused when using DI in a wrong way ...

@mpkorstanje
Copy link
Contributor Author

Cucumber is used by people without a lot of programming experience. So the more Cucumber can explain to them what they are doing wrong, the better.

Anyway, do you have any concrete concerns? Will this impact some other use case in a bad way?

@nkkolluru
Copy link

nkkolluru commented May 30, 2021

Hello, I would like to try this as a first issue, trying to contribute to OSS. Can you give me a starting point in code which will help me get started with this issue @mpkorstanje ?

@laeubi
Copy link

laeubi commented May 30, 2021

At least it will break existing codes without any value, to really help people (at least a basic understand of the used framework (spring here) should be assumed), the cucumber-spring injection-module might better be enhanced to issue a warning/error instead of cucumber-jvm itself.

@mpkorstanje
Copy link
Contributor Author

mpkorstanje commented May 30, 2021

@laeubi I'm looking for a scenario where it might be useful to have static hooks and step definitions -- regardless of the dependency injection container used. Something like #1950, #1953.

@laeubi
Copy link

laeubi commented May 30, 2021

Maybe its more a style thing, but if a method do not reference any instance field I prefer to make it static and as it is already supported why remove this support for little gain?

@mpkorstanje
Copy link
Contributor Author

Fair enough. That makes sense.

@kishore262 I'm afraid you'll have to look at any one of the other issues tagged as help wanted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Defect / Bug good first issue Good for newcomers 🙏 help wanted Help wanted - not prioritized by core team
Projects
None yet
Development

No branches or pull requests

3 participants