Adding a command-line interface, CSV field resolver, and usage examples#15
Adding a command-line interface, CSV field resolver, and usage examples#15chui101 wants to merge 5 commits intoimsweb:mainfrom
Conversation
| * For more details on building Java & JVM projects, please refer to https://docs.gradle.org/8.3/userguide/building_java_projects.html in the Gradle documentation. | ||
| */ | ||
|
|
||
| plugins { |
There was a problem hiding this comment.
I like the idea of providing a CLI entry point to this library, that makes sense to me, even if it's main purpose was to be embedded into another project. But adding an entire Gradle distribution doesn't make sense. I find that confusing (and it might confuse some IDE as well), and I don't want to have to maintain a second distribution in an "examples" folder.
The library itself doesn't compiles the Groovy source code, it only generates it.
The idea is that the process would generate the file into a folder that is a source folder to another (Groovy) project, and then that project would see the changes and re-compile the files into classes and create a final JAR file with those classes. That's how I use that library.
I am fine if you want to use this library differently, in a way that is more convenient to you, but I am not willing to add and maintain a second Gradle distribution for that purpose. That is outside the scope of this library.
If you want, you can create your own GitHub project that demonstrates how to setup a project that uses this library to do a translation (including the compilation); I don't have a problem with that, and you are welcome to reference that project from the readme file you added. That's what I did a long time ago to demonstrate how to use the validation engine (https://github.com/depryf/validation-example). But that will be your project, and you will have to maintain the Gradle distribution that goes with it.
I don't have an issue with keeping the CLI changes, they are good changes. But I never thought a CLI was useful because the library is not very useful without the compilation step, which usually requires it to be embedded into another project. Not to mention that most libraries are released on Maven Central nowadays and this example distribution only provides the compilation and JAR creation, not the distribution.
Let me know what you think of all this.
There was a problem hiding this comment.
Sounds good to me! I will split off the bytecode compilation example into a separate project.
| throw new IOException("Invalid line in mapping file: " + line); | ||
| } | ||
| getMappings().put(Integer.parseInt(mapping[0]), mapping[1]); | ||
| System.out.println(" >> Loaded mapping from " + mappingsFile.getName() + ": " + mapping[0] + "->" + mapping[1]); |
There was a problem hiding this comment.
I think you should use the logging API anywhere you used System.out; it should be similar to how it's done in the MetafileTranslator.
| implementation 'org.xerial:sqlite-jdbc:3.43.2.1' | ||
| implementation 'org.apache.logging.log4j:log4j-api:2.21.1' | ||
| implementation 'commons-cli:commons-cli:1.6.0' | ||
| implementation 'org.apache.logging.log4j:log4j-core:2.21.1' |
There was a problem hiding this comment.
I am not against this change, but I am a bit surprised it was needed. The library uses the logging interface and rely on the caller to provide a logging implementation.
I assume you added this for your new CLI class, which is fine. But that class doesn't directly log anything.
What required the addition of this library?
There was a problem hiding this comment.
I think this is a remnant of when I was trying to resolve a minor slf4j warning. There was an error message without it:
ERROR StatusLogger Log4j2 could not find a logging implementation. Please add log4j-core to the classpath. Using SimpleLogger to log to the console...
I will remove the dependency for now as the message does not affect the functionality.
There was a problem hiding this comment.
I added the log4j-core dependency back after implementing log4j2 output for the CLI.
All output from the CLI now uses log4j2. Additionally, the slf4j logging output used in the staging-library-java dependency are now directed to the slf4j noop logger.
|
|
||
| **Example usage:** | ||
| ```sh | ||
| $ java -jar validation-translation.jar -c config.properties -p WORKING_DIR |
There was a problem hiding this comment.
I don't mean to be a pain about this pull request, sorry!
The CsvFieldResolver class makes sense, it looks great, we can add it.
I am still trying to figure out the rest.
You are providing an example of how to run the CLI; have you tried that example yourself? It won't work because this library depends on other libraries, and the only way to make it a standalone JAR is to make a shadow JAR (or "fat" JAR, or whatever you want to call it). Basically a JAR that contains all the classes of this library plus all the classes of the dependencies. It's doable, but you will need to provide instructions on how to build that fat JAR, which would require a special Gradle task again.
That makes the CLI pretty much unusable, unless it's embedded into another project. But if it's embedded, then you can simply copy its code into that project and not require a CLI class in this project.
So again, my question to you is: are you using the CLI class yourself, and if you are, how do you call it? From the command line, but you need dependencies. From an other project, but you could just have the CLI class in that project.
I guess what bothers me the most is that you are adding a dependency to a logging implementation. It might not look like much, but it's rally not a great thing to do for a library that is supposed to abstract its logging via just an API and let the parent project deal with whatever implementation they want. This little CLI class makes this project "not really a library", which is fine, except that IMO the CLI class is not useable as-is. And I would prefer to not jump through hoops to make that work unless you are telling me that you need things to be this way.
My preference would be to only keep the CsvFieldResolver and nothing else; it's a great addition to the library.
If you feel strongly about keeping the CLI class, then I would like you to move it to the test package, and put back the logging implementation at the test level so it's not released. You will also need to adjust the these instructions in the README file because those examples won't work as-is.
A third option is for you to move the CLI class and the examples in your other project where you plan on moving the compilation stuff that you removed earlier.
Plenty of options, and we can certainly discuss this more if you don't agree with my point-of-view.
There was a problem hiding this comment.
No worries. Sorry for the delay, we are deep in 2024 changes...
I have run the CLI to translate the NCDB edits several times now, so I know it works! I originally modified the jar task to build a fat jar, but I have now split that off into a separate task, 'buildFatJar'. I will do some further work to move the CLI into the test package to prevent log4j from cluttering up the dependency tree.
There was a problem hiding this comment.
I agree it will work with a fat JAR. It wasn't clear to me that "validation-translation.jar" was supposed to be a fat JAR in your example:
$ java -jar validation-translation.jar -c config.properties -p WORKING_DIR
And that's kind of the whole argument for this; the full translation process (which includes creating the runtime classes) is really meant to be part of another project that takes care of compiling the runtime classes. And if this project can't do the compilation, then why adding the rest (the CLI and the fat JAR)? IMO, those two things should be part of your other project. They are part of a "full translation" that requires another project setup.
It's hard to explain.
My preference would be for you to move the CLI and the fat JAR generation in your other project. The only change that really makes sense for this library is the CSV field mapper.
If you feel strongly about keeping the CLI and JAR generation in this project, that's fine, as long as log4j is not brought up into the released dependencies. There have been too many security issues with it in the past few years...
|
@chui101 I think I might have deleted this branch by mistake. Sorry. I was doing some house keeping and saw a branch that I didn't recognized and was there for a very long time. I thought it was one of mine and I removed both my local and the remote version of it. Hopefully you still have your local version somewhere. On the other hand, we never really resolved this request, and I would rather not add too much complexity to this project for your use case. I would prefer that you wrap this library into your own (and if you need hooks to make it work, that would be fine). |
2 participants
No description provided.