-
-
Notifications
You must be signed in to change notification settings - Fork 355
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
feature: add fluent API for Launcher #3258
Conversation
ohoh, welcome FluentLauncher!
|
*/ | ||
public class FluentLauncher { | ||
|
||
Launcher launcher; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be private
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes 100%.
return this; | ||
} | ||
|
||
public void binaryOutputDirectory(File outputDirectory) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to follow the fluent interface idiom?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes needs to be changed.
} | ||
|
||
// needed??? | ||
public SpoonModelBuilder compiler(Factory factory) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does a SpoonModelBuilder
differ from the FluentLauncher
? Conceptually, to me, they seem like they are trying to accomplish the same task: using a builder pattern to configure Spoon. Exposing both may lead to some confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SpoonModelBuilder interface doesn't expose a fluent api.
thank you for the review, but the code here is just a draft and far from even worth reading. There are still settings missing, unneeded methods and maybe it's possible to wrap all 3 launcher in the same class. Finding a better name is open too. |
Thanks for starting this @MartinWitt! So I'd just start with some easy well-defined use cases and iterate over them in the launcher. So basically remove everything with the "needed" question, and everything's out of the scope of the usecases expressed in #1563. It should drastically reduce the size of the class. The idea is to have a first launcher to fit 90% of usecases, for other option users will still have the old non-fluent API and we can improve it later to add ways to access those options. |
as @surli suggested, i made the api slim.
Are there any imported options someone is missing? |
I'm +1 to add a support for MavenLauncher since it's already an easy usecase for Spoon and it would show how to extend the capabilities of the FluentLauncher with another existing launcher. |
There were 2 options for adding the mavenlauncher. |
Looks great, we just need a test case, the examples you had are great, could you add them back? |
That's really cool! +1 for a small unit test, could be done with a mock for the wrapped launcher. |
Maybe #3259 is not compatible with the spoon-control-flow code? The CI failure
seems unrelated to this feature. See #3271 for potential fix. |
(just for info in case you don't know: you shouldn't need a commit to re-run the CI: triggering Travis from the UI is enough since it performs the build by merging changes with master each time) |
Thanks a lot @MartinWitt |
Showcase some code for discussion #1563 . There are still a lot of calls to the environment missing. It's only to showcase the idea.