-
Notifications
You must be signed in to change notification settings - Fork 134
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
Remove afterEvaluate from baseline immutables #1752
Conversation
Generate changelog in
|
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.
Thanks for adding at test!
import org.gradle.api.tasks.compile.JavaCompile; | ||
|
||
public final class BaselineImmutables implements Plugin<Project> { | ||
|
||
@Override | ||
public void apply(Project project) { | ||
project.getPluginManager().withPlugin("java", unused -> { | ||
project.afterEvaluate(proj -> { | ||
proj.getConvention().getPlugin(JavaPluginConvention.class).getSourceSets().stream() |
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.
Calling .stream()
on a DomainObjectCollection
is pretty much always a bug, as it doesn't include objects added later (and encourages people to use afterEvaluate). I wonder if we should make this an error prone check?
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.
Sounds like a great idea
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.
That said, we do use a .stream()
below correctly....
proj.getConvention().getPlugin(JavaPluginConvention.class).getSourceSets().stream() | ||
.filter(sourceSet -> hasImmutablesProcessor(project, sourceSet)) | ||
.forEach(sourceSet -> addImmutablesIncrementalCompilerArg(project, sourceSet)); | ||
project.getExtensions().getByType(SourceSetContainer.class).configureEach(sourceSet -> { |
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 we use .matching(...)
here to keep this fluent?
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.
Hmm, what would we match on here?
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.
hasImmutablesProcessor
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.
This is what we do in some other plugins.
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.
I don't think this will be as lazy as it can be, as we currently check for immutables the moment the compiler args are requested, where as with .matching
on the source sets would check for immutables the earliest of when each source set is created or when configureEach
is called.
Also just be aware that there's a lot of really old stuff in this repo and a lot of it is a bit dodgy (but works well enough)
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.
I tend to avoid fluent code because it makes the behavior less obvious to readers, especially as ordering is concerned.
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.
Cool, make sense. Thanks for the explanation!
Released 3.81.1 |
Before this PR
Thanks to @pkoenig10's hard work, we have enabled incremental compilation of immutables classes in #1750! However, it uses
afterEvaluate
, which in my experience nearly always leads to pain down the road when gradle scripts get complicated and multipleafterEvaluate
s are run with no defined order.After this PR
==COMMIT_MSG==
Build compiler args for
com.palantir.baseline-immutables
plugin lazily to avoidafterEvaluate
ordering issues.==COMMIT_MSG==
Additionally add a test.
Possible downsides?