My Approach for "Support Provider instances with Pico Container"

[antagoony]

🤔 What's changed?

This is my approach to solve #2879.

⚡️ What's your motivation?

Just saw the "help wanted" label, plus it sounds challenging ;-)

I read your thoughts and agree most of it. I do not totally follow the argument to "[...] declare a proper factory object". I think if any user decides to use PicoContainter he most likely wants to see it working as expected by this CI framework. Similarly Cucumber does not confuse Spring users. There is no new injection approach different to the Spring way of doing so.

Admittedly Cucumber-Spring does not automatically scan for all Spring components. Instead it scans exactly where the user wants it to do (by providing further Spring annotations on exactly that class that holds the @CucumberContextConfiguration annotation).

Following this idea, my PR comes up with something similar. It provides a @PicoConfiguration annotation that can be used to declare any Provider and/or ProviderAdapter classes that shall be used.

🏷️ What kind of change is this?

• ⚡ New feature (non-breaking change which adds new behaviour)

♻️ Anything particular you want feedback on?

Not particular but in general -- what do you think of this approach?

📋 Checklist:

• I agree to respect and uphold the Cucumber Community Code of Conduct
• I've changed the behaviour of the code I've added new code
  • I have added/updated tests to cover my changes.
• My change requires a change to the documentation.
  • I have updated the documentation accordingly.
• Users should know about my change
  • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

[mpkorstanje]

I do not totally follow the argument to "[...] declare a proper factory object". I think if any user decides to use PicoContainter he most likely wants to see it working as expected by this CI framework.

Driving the desire for conceptual simplicity is the fact that the Cucumber project recommends Picocontainer if no other competing default exists (i.e. the user isn't already using Spring). And a good number of users are relatively novices at Java. So we've taken care to sand down the proverbial sharp edges by limiting the supported use-case to constructor dependency injection.

Looking back, I think I was hoping for an equally elegant solution, but none presented itself. But I can see myself explaining that users should use constructor dependency injection for glue code they write and providers for code they don't.

Anyway. The current pull request is definitely in the right spirit! And I think the biggest challenge is design related. Unfortunately, I don't have too much time right now. So below are some quick notes, apologies for the unvalidated train of thought.

1. I don't think that the ProviderAdapter is a good first candidate for this feature. The class is hairy. It might be possible to use without any chance of accidental misuse but I simply don't have the time to consider it. So for now I'd like to leave it out of scope. The Provider is a good candidate though.

2. The @PicoConfiguration could be replaced with a @CucumberPicoProvider (see example below).
   I understand that @PicoConfiguration allows the providers to be anywhere, even outside of the glue path. But I've recently drafted (#3120) and while unfinished I think a @PicoConfiguration could inevitably evolve into a more generic set of @UseGlueClasses, @UseGluePackages annotations that would allow adding arbitrary classes and packages to the glue. We might even have to undeprecate the @Cucumber annotation from JUnit 5 for that.

package com.example.app;

import java.sql.*;
import io.cucumber.picocontainer.CucumberPicoProvider;
import org.picocontainer.injectors.Provider;

public class ExamplePicoConfiguration {

    @CucumberPicoProvider
    public static class DatabaseConnectionProvider implements Provider {
        public Connection provide() {
            return // something
        }
    }

}

3. On reflection, the inner class looks quite similar too what I originally had in mind with. So I think that would be the more elegant way to go, there is much less fluff involved.

public class WebDriverFactory {

   @CucumberPicoProvider
   public static WebDriver webDriver() {
       return // create WebDriver 
   }

    @CucumberPicoProvider
    public static SecuredPage provide(WebDriver driver, Project project) {
       return new SecuredPage(driver, project, "example", "top secret");
    }
}

Which runs into my last point from #2879:

But at point we may as well recommend that people use a fully featured DI such as Spring or Guice. Perhaps what we need instead are some good examples.

I you're willing to make a pull request, I'm sure I can step over that. 😄

So in summary, both the @CucumberPicoProvider annotated Provider class and the @CucumberPicoProvider annotated method are acceptable solutions. I'll leave the choice up to you depending on your desire for a challenge.

If you any have suggestions for improvement, happy to hear them as always!