mariofusco commented on code in PR #5765:
URL:
https://github.com/apache/incubator-kie-drools/pull/5765#discussion_r1517454961
##########
drools-model/drools-codegen-common/src/main/java/org/drools/codegen/common/AppPaths.java:
##########
@@ -47,6 +47,7 @@ public static AppPaths.BuildTool findBuildTool() {
private final boolean isJar;
private final BuildTool bt;
private final Path resourcesPath;
+ private final Path generatedResourcesPath;
Review Comment:
> I look at the code. It would be possible to do as per your suggestion, but
IMO it would be unconsistent and more cumbersome. About consistency:
`generatedResourcesPath` are semantically the same as `resourcesPath` ; and
both depends on the `bt` status: it make sense to manage them exactly in the
same way/same place
They are probably semantically similar, nevertheless while `resourcesPath`
is used in 2 distinct methods (see also `getResourceFiles()`), this is not the
case for the `generatedResourcesPath`, so while it is necessary to make the
first a class instance variable, this does not applies for the second.
> About cumbersomeness I would need to introduce this check -> `if (bt ==
BuildTool.GRADLE) {` inside the `getResourcesPAths` - but this check is already
executed in the constructor
You're having that if anyway doing `if (generatedResourcesPath != null)`. I
don't see how replacing it with `if (bt != BuildTool.GRADLE)` would make the
code more cumbersome.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
