-
Notifications
You must be signed in to change notification settings - Fork 79
Add ability to use conversions and type factories residing outside of build classpath #228
Add ability to use conversions and type factories residing outside of build classpath #228
Conversation
Hey, @erdi, nice to hear from you. I'll take a look, and assuming I don't find any issues we can't work past, am fine with merging and releasing. Glad the plugin is helping people. :-) |
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.
Code changes look good.
Could you please take a pass at adding documentation of the new configuration options to the README.md, and a line to the unreleased changes section of CHANGES.md?
With this new approach, do you think that there are any of the previous configuration options that no longer make sense that should be deprecated or removed?
Based on the CI job, looks like some currently supported Gradle versions don't support injection of Project into extensions like you used.
Please consider if there are other options that would be compatible. If not, determine what the minimum Gradle version needed for this feature is, and I'm okay with updating the minimum supported version of Gradle. |
Should be fixed in 5ede65a, waiting for the CI job to confirm... |
…tside of build classpath
Addressed in 09fafb2
While everything that could be previously achieved with |
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.
Changes so far look good. Just the deprecation notice and a clean CI run and I'll be ready to merge.
…actories with classes
Deprecations added in b4ac5ba. All comments addressed, over to you. |
@erdi Thanks! I'll review the CI results, and unless something new comes up, will merge and hopefully do a release sometime this evening. |
CI looks green. 🎉 Fingers crossed nothing new comes up and you will find a while to do the release. Thanks for prompt reviews! |
Included in release 1.7.0 and published to Maven Central. There may be some delay before it becomes visible there. https://github.com/davidmc24/gradle-avro-plugin/actions/runs/4620107604/jobs/8169751045 |
Awesome, thank you very, very much! |
FWIW, I've integrated these changes into the project I'm using the plugin on and everything works as expected. 🎉 |
@erdi Would you be able to share an example project. I have been trying hard to make this work but not successful. |
For an example project I'd suggest you check out the test I added as part of this PR, @anilkulkarni87. I'd debug the test, put a breakpoint at https://github.com/davidmc24/gradle-avro-plugin/pull/228/files#diff-b6d697ce9c603d8ed2be90b0987c81490d2bc8d30c8759a33906af5e492558f4R207, evaluate |
@erdi Thank you for the response. But i was able to make it work. https://github.com/anilkulkarni87/AvroJnaana/blob/main/schemas/build.gradle This made life so much easier for me. |
I’m glad to hear that this is beneficial to more people than just myself, @anilkulkarni87. |
This is needed for when the conversions and/or type factories that are being configured for use by the plugin are part of the same build and you end up with a chicken and egg problem forcing you to using trickery in
buildSrc
(this is actually exposed by existing tests inCustomConversionFunctionalSpec
). Hopefully the test I added documents the use case and proves that the changes are working as intended.I tried hard for this to be up to scratch. I did a basic compatibility check (thanks for making that easy, btw) which forced me to change my initial implementation slightly to account for Gradle 5.1. I will keep my eye on the pipeline and address any failures.
Please be informed that I will address any feedback within one business day at the most. I would really like to have this merged and released so I will gladly collaborate on making this happen.
I know this project is in maintenance mode, @davidmc24, so I'm sorry to be opening a PR but it already mostly does the job for us apart from the issue addressed by this PR so I'd rather contribute than reinvent the wheel.