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

add ability to pass options via command's constructor #1107

Closed
wants to merge 1 commit into from

Conversation

eitansuez
Copy link

hi remko,
i'd like you to review this commit.
the intent here is to support passing options via the constructor.

i don't think it should be merged as is, without your review and perhaps some tweaks, as i don't fully understand all the magic that picocli does.

this code change causes one of the subcommandtests to fail, but i hope that's something that's easily rectified.

i definitely need to spend more time understanding the code.

my main reflection at the moment is that i suspect spring does similar work when it builds application contexts. i wonder if this project can leverage the same utilities that spring uses to build the command object, leveraging autowiring, constructor and setter injection so we don't have to.

@remkop
Copy link
Owner

remkop commented Jun 12, 2020

i definitely need to spend more time understanding the code.

Yes, I worked on this area of user object instantiation recently when implementing [#690], and when refactoring how default values are applied ([#962][#961][#990]). I am keenly aware that user object creation is one of the most complex areas of the library. I am not sure if this is because of the implementation or whether it is because of the nature of the problem.

This is why I mentioned the trade-off between the benefit that this feature would bring vs the increased maintenance burden of the additional complexity.

my main reflection at the moment is that i suspect spring does similar work when it builds application contexts. i wonder if this project can leverage the same utilities that spring uses to build the command object, leveraging autowiring, constructor and setter injection so we don't have to.

This may be interesting to explore. picocli itself should not require Spring to work, but there is a picocli-spring-boot-starter module, and one idea may be to support constructor injection for applications that use that module.

Speaking of Spring and dependency injection, picocli currently supports integration with dependency injection containers via the IFactory interface. This allows DI containers to inject services and beans into subcommands, type converters, and other components.

If our constructor injection implementation instantiates subcommands via reflection, without using the factory, then such subcommands that pass options to the constructor may not be able to get other services and beans injected by the dependency injection container... That would be a big drawback. Supporting constructor injection via the PicocliSpringFactory may avoid that problem.

@eitansuez
Copy link
Author

you are correct. the IFactory.create(cls) method needs to be revised to include one more parameter: the commanSpec's constructorParams field, which becomes necessary to call the constructor with parameters. i dodged making this change to avoid the cascade of changes to every client of IFactory, but that's clearly not right. so this PR needs more work.

@remkop
Copy link
Owner

remkop commented Jun 13, 2020

We cannot break existing factories.

To be honest, I am having second thoughts about this. Is it going to be worth it? How valuable is constructor injection in our use case?
I am less and less convinced that this is a good trade-off.

@eitansuez
Copy link
Author

i suppose i can create a type of factory where the initialization parameters are passed in when the factory is built, and then the create() method would not need to be modified. in that way the interface would not change. but i agree with you, i don't want to introduce a chance to the codebase that makes it more brittle. let's abandon this PR altogether. thanks for entertaining it.

@remkop
Copy link
Owner

remkop commented Jun 14, 2020

That makes sense.
Agree to abandon this effort.
Thank you for exploring this!

@remkop remkop closed this Jun 14, 2020
@remkop remkop added the status: declined ❌ A suggestion or change that we don't feel we should currently apply label Jan 25, 2021
@remkop remkop mentioned this pull request Apr 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined ❌ A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants