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 input/output annotations to transformers #646

Closed
wants to merge 7 commits into from

Conversation

wolfs
Copy link
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
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
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
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
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
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