Handle plugins block and empty url in gradle source #159
Conversation
Most more advanced build.gradle files out in the wild has a plugins block, buildscript block or both. At the moment there is a restriction where plugins blocks can be added, so the current implementation cant handle the "apply build.gradle" clause when generating the tempfile. This fix will check if the "Gradle license report" plugin is installed, and if not, either add it to the plugins block, or add an plugins block with it if there was none in the build.gradle file. This should be compatible with all the cases first mentioned. Also, there was a problem where if one dependency was missing an url, the check would fail with exception. This takes an easy way out to fix this - it sets it to an empty mutable string.
|
I tested the fix on a few real life projects with the above mentioned blocks |
There was a problem hiding this comment.
This is great! Thank you for the bug fix 🙇 I've got a couple minor requested changes/reverts
I'm also interested in whether you can add or update the gradle source tests or fixtures to verify that the fixes work and that regressions aren't introduced in the future. I have minimal exposure to gradle so I'd rely on your knowledge on how to craft those tests, if possible
|
I agree with your comments. Sorry for the formatting, and for forgetting the tests, I did this patch on a train ride and stressed to get it done before I reached home. I'll try to get the fixes done on friday, when we have our hack-on-whatever-you-want day. And of course, if anyone else should feel like doing the fixes before that, they are all welcome. |
|
A question about test fixture(s) - how detailed do you want them - should I go for having all the variants we want to test in different fixture files. 1) no buildscripts or plugins block (like now) 2) only plugins block, no license dep added 3) only plugins block, license dep added and so on. with every permutation/possible combination it would be quite a few tests. I interpret the fixtures to be more of a high perspective integration tests though, and as a first step add the ones I think is most important. A few tests is better than no tests. |
|
@jandersson-svt
Not a problem at all! Thank you for trying out the new source and putting together a PR to fix the issues you found 🙇
✨ this sounds great. I'm not terribly concerned about covering all edge cases right now. Getting coverage for common gradle files patterns would be perfect. |
|
Let's try this. Fixed all your suggestions as wished, I only did at simple addition to the build.gradle file for testing the pattern with a plugins block and a buildscript block. I'll add more later, but hope this basic is enough for this PR. Btw, me and my colleague have some ideas and some suggestions, should I make an issue for each so that it can be discussed there? (I also found one bug, but I think that is in one of the helper libraries, I'll have a look before making an issue or PR) |
Most more advanced build.gradle files out in the wild has a plugins block,
buildscript block or both. At the moment there is a restriction
where plugins blocks can be added, so the current implementation
cant handle the "apply build.gradle" clause when generating the
tempfile.
This fix will check if the "Gradle license report" plugin is installed,
and if not, either add it to the plugins block, or add an
plugins block with it if there was none in the build.gradle file.
This should make the gradle source compatible with all the cases first mentioned.
Also, there was a problem where if one dependency was missing an url,
the license cache check would fail with exception.
This takes an easy way out to fix this - it sets it to an empty
mutable string.