Skip to content
This repository has been archived by the owner on Dec 28, 2023. It is now read-only.

Support specifying classpath for additional velocity tool classes #222

Merged
merged 2 commits into from
Jan 31, 2023

Conversation

crtlib
Copy link
Contributor

@crtlib crtlib commented Jan 30, 2023

Hi. Needed this for instantiating some custom non-published velocity tool classes. Would be great to have it released soon 🙏

@davidmc24
Copy link
Owner

Thanks for the PR. I'm open to this change, but don't want to break compatibility for it. If you resolve that issue with test coverage to prove compatibility, I should be able to merge and release relatively soon afterward.

@crtlib
Copy link
Contributor Author

crtlib commented Jan 30, 2023

Thanks, David. Indeed, my change broke the supports configuring velocity tool classes in OptionsFunctionalSpec. I just force-pushed a change, so that we now specify the parent classloader in our custom one. That fixed the tests. Does it mean that the current tests already cover the needed compatibility? If not, can you please elaborate on what kind of test you have in mind? Thank you!

@davidmc24
Copy link
Owner

Yes, I'd thought that there was coverage for additional velocity tools. I try pretty hard not to add features without test coverage. If it failed before and passes now, that's probably good enough. We'll just need test coverage for the new case, where the classpath is configured (if you haven't added that already)

@crtlib
Copy link
Contributor Author

crtlib commented Jan 31, 2023

Hi, David. Looks like I've managed to deliver a test for the new property 😅 Please take a look 🙏

@davidmc24
Copy link
Owner

Looks good, with minor tweaks that I'll take care of post-merge. I'll merge now; assuming that the CI run looks clean I'll pursue a release in the next day or two.

@davidmc24 davidmc24 merged commit e69bbe7 into davidmc24:master Jan 31, 2023
@davidmc24 davidmc24 added this to the 1.6.0 milestone Jan 31, 2023
@crtlib
Copy link
Contributor Author

crtlib commented Jan 31, 2023

@davidmc24 Thanks a lot 🙏

@davidmc24
Copy link
Owner

@crtlib Looks like the new test (and maybe functionality) isn't Windows compatible. Mind trying for a fix?

https://github.com/davidmc24/gradle-avro-plugin/actions/runs/4055083756/jobs/6977738565

@crtlib
Copy link
Contributor Author

crtlib commented Feb 1, 2023

@davidmc24 Sure, David. I'll give a try.

@crtlib
Copy link
Contributor Author

crtlib commented Feb 1, 2023

@davidmc24 Here it is: crtlib@e778f21. Should I create a PR or you'll just cherry-pick?

@davidmc24
Copy link
Owner

PR if possible, please

@davidmc24
Copy link
Owner

@crtlib Release underway. Once https://github.com/davidmc24/gradle-avro-plugin/actions/runs/4104821988 completes, it should be published to Maven Central as version 1.6.0.

@davidmc24
Copy link
Owner

Released

@crtlib
Copy link
Contributor Author

crtlib commented Feb 6, 2023

@davidmc24 Thank you so much 🙏

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants