Allow attributes to be defined on plugin dependencies #28981
Conversation
jvandort
force-pushed
the
jvandort/iss/28933
branch
from
275e89f
to
a4e5091
Compare
jvandort
force-pushed
the
jvandort/iss/28933
branch
from
a4e5091
to
ad9e6b6
Compare
jvandort
force-pushed
the
jvandort/iss/28933
branch
2 times, most recently
from
61b0ead
to
66d072c
Compare
Use a DependencyFactory to instantiate dependency instances generated by plugin requests so that they have their services properly injected. Previously, they were instantiated directly and did not have their services. Also remove the ArtifactRepositoriesPluginResovlerTest, as it relied heavily on mocks and therefore did not reflect the actual environment that the class-under-test was operating under. The tests it performed were already addressed by existing integ tests.
jvandort
force-pushed
the
jvandort/iss/28933
branch
from
66d072c
to
894fac1
Compare
|
I've triggered the following builds for you. Click here to see all build failures. |
| } | ||
| } | ||
|
|
||
| private String getNotation(Dependency dependency) { | ||
| return Joiner.on(':').join(dependency.getGroup(), dependency.getName(), dependency.getVersion()); | ||
| } | ||
|
|
||
| private DependencyFactory getDependencyFactory() { |
There was a problem hiding this comment.
🤔 I think it would be better to story the DependencyFactory as a field in the constructor, rather than having to call this private getter everytime. That would also remove the need for the resolutionServices field.
There was a problem hiding this comment.
ResolutionServices is already used elsewhere in this class. Do you recommend pulling out the other two used services as well?
There was a problem hiding this comment.
I missed it being used in the folded unchanged section. Probably not worth removing removing the one field to add two others then. Still, I think you can avoid the getter.
| @@ -648,6 +649,11 @@ public AttributesSchema getAttributesSchema() { | |||
| public ObjectFactory getObjectFactory() { | |||
| return services.get(ObjectFactory.class); | |||
| } | |||
|
|
|||
| @Override | |||
| public DependencyFactory getDependencyFactory() { | |||
There was a problem hiding this comment.
💭 Kind of odd this wasn't already present here.
| } | ||
| } | ||
|
|
||
| private String getNotation(Dependency dependency) { | ||
| return Joiner.on(':').join(dependency.getGroup(), dependency.getName(), dependency.getVersion()); | ||
| } | ||
|
|
||
| private DependencyFactory getDependencyFactory() { |
There was a problem hiding this comment.
I missed it being used in the folded unchanged section. Probably not worth removing removing the one field to add two others then. Still, I think you can avoid the getter.
|
WARN: Based on labels, this pull request addresses notable issue but no changes to release note found. |
|
@bot-gradle test this |
|
I've triggered the following builds for you. Click here to see all build failures. |
|
WARN: Based on labels, this pull request addresses notable issue but no changes to release note found. |
Fixes #28933
Reviewing cheatsheet
Before merging the PR, comments starting with