Skip to content

Add input/output annotations to transformers#646

Closed
wolfs wants to merge 7 commits intoGradleUp:masterfrom
wolfs:transformer-annotations
Closed

Add input/output annotations to transformers#646
wolfs wants to merge 7 commits intoGradleUp:masterfrom
wolfs:transformer-annotations

Conversation

@wolfs
Copy link
Copy Markdown
Contributor

@wolfs wolfs commented Mar 12, 2021

so that the transformer remain compatible with Gradle 7.0.
In Gradle 7.0 missing annotations on input properties will fail the build. Input properties include properties on @Nested inputs, which includes all the Transformers.

@wolfs wolfs force-pushed the transformer-annotations branch from da350bc to 206ce6e Compare March 15, 2021 17:47
wolfs added 2 commits March 15, 2021 18:58
so that the transformer remain compatible with Gradle 7.0.
In Gradle 7.0 missing annotations on input properties will fail the
build. Input properties include properties on `@Nested` inputs,
which includes all the Transformers.
@wolfs wolfs force-pushed the transformer-annotations branch from 206ce6e to 75478c5 Compare March 15, 2021 17:59
Copy link
Copy Markdown
Contributor

@melix melix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@helfper
Copy link
Copy Markdown
Contributor

helfper commented Mar 18, 2021

I hadn't seen this before. Interesting that we came up with very similar solutions independently (#647). I believe you beat me to it with the time I spent figuring out why the test was not working for ManifestResourceTransformer and then fixing it upstream in https://github.com/apache/ant.

I think the main differences are:

  • I preferred to make fields private instead of annotating them with @Internal
  • Making Transformer implement Named shows the name of the transformer in the warnings
  • Asserting that there's no deprecation message in the output line by line instead of the whole output makes it easier to find the offending line when it fails

@wolfs
Copy link
Copy Markdown
Contributor Author

wolfs commented Mar 29, 2021

@helfper I looked at your PR and it looks good! I left some comments regarding the annotations. I still would like to salvage the changes I did here to assert that no test has any deprecation warnings. I'll extract this into another PR and then close this one. Or do you prefer that I base this PR on top of yours?

@wolfs wolfs mentioned this pull request Mar 29, 2021
@wolfs
Copy link
Copy Markdown
Contributor Author

wolfs commented Mar 29, 2021

@helfper I extracted the assertion code to #654. There I also assert line-wise like you did.

@johnrengelman
Copy link
Copy Markdown
Collaborator

Closing in favor of #647 and #654 per the conversation above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants