Fix Gradle add extension and add tests #4614
Conversation
|
I abstracted MavenExtensionsTest so we have the same coverage for Gradle. Note that one test is not passing with Gradle and I think it should come in another issue/PR: |
| protected boolean containsBOM() { | ||
| return buildContent.contains("enforcedPlatform(\"io.quarkus:quarkus-bom:"); | ||
| protected boolean containsBOM() throws IOException { | ||
| return getModel().getBuildContent().contains("enforcedPlatform(\"io.quarkus:quarkus-bom:"); |
There was a problem hiding this comment.
although G:A:V syntax is more common, people can also declare gradle dependencies with expanded syntax like this:
group: 'io.quarkus', name: 'quarkus-bom', version: '0.25.0'with any amount of spacing or newlines in between
There was a problem hiding this comment.
Right, not mentioning the code formatting could vary. This functionality needs to be re-written properly.
| StringBuilder newBuildContent = new StringBuilder(); | ||
| readLineByLine(buildContent, new AppendDependency(newBuildContent, dependency)); | ||
| buildContent = newBuildContent.toString(); | ||
| readLineByLine(getModel().getBuildContent(), new AppendDependency(newBuildContent, dependency)); |
There was a problem hiding this comment.
while it's not an issue with this PR specifically, I was looking into the code for AppendDependency and I noticed 2 corner cases that don't currently work:
- It looks for
line.startsWith("dependencies {"which doesn't account for additional spacing betweendependenciesand{and also doesn't account for leading whitespace (which is normal if this is being applied in asubprojects{}block for example). - If we have the quarkus plugin installed already, but no
dependencies{}at all, nothing gets added. For this case I think we should probably add the quarkus-bom dependency with a version corresponding to the quarkus-gradle-plugin, followed by the rest of the requested extension dependencies as normal
There was a problem hiding this comment.
TBH I didn't really look at the business involved.. I just fixed the bug using the same system used in MavenBuildFile and added the tests that should have been there in the first place..
There was a problem hiding this comment.
At some point we are going to rewrite most of the codegen part with a real template engine for Quarkus that is beeing developed https://github.com/quarkusio/qute.
But for now, any corner case you see, please feel free to provide a PR with some tests :)
There was a problem hiding this comment.
Yeah this part of the code is messy currently... So trying to improve little things probably doesn't make a ton sense, tests on the other hand definitely do :).
@ia3andy did you sign up for rewriting the part this qute? :P
There was a problem hiding this comment.
@geoand why not, that might be fun, though for now I have plenty on my plate 😅
|
@ia3andy seems like the JVM tests are failing |
|
@gsmet we need that merged asap too |
4 participants
Closes #4601