quarkus-camel-catalog #242
Conversation
|
Refer to this link for build results (access rights to CI server needed): |
There was a problem hiding this comment.
Some questions and proposals inline.
@davsclaus could you please squash the commits into one to have a better readable history? Less commits are also easier to review locally an IDE.
| @@ -0,0 +1,203 @@ | |||
|
|
|||
There was a problem hiding this comment.
There are LICENSE.txt and NOTICE.txt files in the root directory of this repo. What is the reason to duplicate them here? I mean this module could pull them from the root directory. That would be less repo space and less risk that the files go out of sync. I hope I am assuming right that all code in the current repo should be covered by the same notice and license provisions?
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.apache.camel.catalog.quarkus; |
There was a problem hiding this comment.
Using org.apache.camel.quarkus.catalog namespace would perhaps be safer to avoid potential naming clashes (unless there is a plan to move this code to where org.apache.camel.catalog is hosted?)
| <version>0.2.1-SNAPSHOT</version> | ||
| </parent> | ||
|
|
||
| <artifactId>catalog</artifactId> |
There was a problem hiding this comment.
I vote for having camel-quarkus- prefix on all artifactIds in this repo for practical reasons:
- Eclipse requires workarounds to have multiple modules with the same artifactId in the same workspace
- Copying jars into one directory (e.g. to build flat class path) requires special caution if the jar names are not unique.
| <version>0.2.1-SNAPSHOT</version> | ||
| </parent> | ||
|
|
||
| <artifactId>tooling-parent</artifactId> |
There was a problem hiding this comment.
I propose to move the significant parts from here to tooling/pom.xml and remove this Maven module. Less modules means a faster build. Actually unless there are any immediate plans to have more tooling modules here, we could even merge all current tooling modules into a single maven plugin module under the existing build directory. WDYT?
| <description>Camel Quarkus Tooling Parent POM</description> | ||
|
|
||
| <properties> | ||
| <maven-version>3.6.2</maven-version> |
There was a problem hiding this comment.
This project uses <id>.version naming convention for the version properties.
| @@ -0,0 +1,203 @@ | |||
|
|
|||
There was a problem hiding this comment.
A license and notice files duplicated once again.
| @@ -0,0 +1,16 @@ | |||
| ## --------------------------------------------------------------------------- | |||
There was a problem hiding this comment.
Is this empty properties file really needed?
| */ | ||
| public static String loadText(InputStream in) throws IOException { | ||
| StringBuilder builder = new StringBuilder(); | ||
| InputStreamReader isr = new InputStreamReader(in); |
There was a problem hiding this comment.
I am not sure what possible resources is this going to read but using new InputStreamReader(in) that uses the given platform's default encoding does not seem to be right? I mean `new InputStreamReader(in, 'utf-8') or similar is perhaps a better bet?
There was a problem hiding this comment.
If new InputStreamReader(in) throws an exception, then in is not closed. Could you please enclose all closeables in a try-with?
| } | ||
|
|
||
| public static void writeText(File file, String text) throws IOException { | ||
| FileOutputStream fos = new FileOutputStream(file, false); |
There was a problem hiding this comment.
Could you please use try-with to have less lines of code?
| public static void writeText(File file, String text) throws IOException { | ||
| FileOutputStream fos = new FileOutputStream(file, false); | ||
| try { | ||
| fos.write(text.getBytes()); |
There was a problem hiding this comment.
String.getBytes() uses the given platform's default encoding. text.getBytes('utf-8') or similar would perhaps be a better choice?
|
Refer to this link for build results (access rights to CI server needed): |
Fixes #75: Adding camel-quarkus-catalog
This implementation is similar to what we do at apache camel using maven plugins as tools to generate catalog metadata that gets included in the camel-quarkus-catalog JAR.