Extract CLI Java components into new submodule

abelsromero commented 1 year ago

Kind of change

[ ] Bug fix
[x] New non-breaking feature
[ ] New breaking feature
[ ] Documentation update
[ ] Build improvement

Description

What is the goal of this pull request?

Extract cli components into it's own submodule. The main motivation is avoiding downloading jCommander for users of Asciidoctorj. In terms of jar size it's not reducing much.

How does it achieve that?

Create new sub-module asciidoctorj-cli: migrated 'org.asciidoctor.jruby.cli' classes and related tests to it.
Created CliOptions in AsciidoctorUtils from values in AsciidoctorCliOptions to avoid circular dependency.
Updated README with new sub-module information.
Fix jar names in command in README.
Minor refactors and fixes.

Are there any alternative ways to implement this? There's an opinionated decision of exposing asciidoctorj & asciidoctorj-api as api so the cli module doesn't need to require them one by one. Semantically it's consistent imo, but I am open to debate.

Are there any implications of this pull request? Anything a user must know? If someone is using AsciidoctorInvoker, they'll need to add new new asciidoctorj-cli as dependency.

Issue

If this PR fixes an open issue, please add a line of the form:

Fixes #Issue Fixes #246

Release notes

Please add a corresponding entry to the file CHANGELOG.adoc

abelsromero commented 1 year ago

A few extra questions...

Should ext.publicationName have some specifc value? I don't think it's necessary but Gradle complains if it's missing.
test more_than_one_input_file_should_throw_an_exception does not make sense. Maybe the name is wrong?
Seems to me only AsciidoctorInvoker should be in package org.asciidoctor.jruby.cli, the rest don't depend on jRuby implementation and could be moved to org.asciidoctor.cli?

robertpanzer commented 1 year ago

Great idea! The maven or gradle plugin shouldn't have to download jcommander.

Should ext.publicationName have some specifc value? I don't think it's necessary but Gradle complains if it's missing.

Yes, it would be great to have sth like this:

project.ext.publicationName = "mavenAsciidoctorJCli"

Right now the name conflicts with the publicationName of the asciidoctorj module. If you run ./gradlew tasks you can see that every publication has its own publishing task:

publishAllPublicationsToLocalRepository - Publishes all Maven publications produced by this project to the local repository.
publishAllPublicationsToSonatypeRepository - Publishes all Maven publications produced by this project to the sonatype repository.
publishMavenAsciidoctorJApiPublicationToLocalRepository - Publishes Maven publication 'mavenAsciidoctorJApi' to Maven repository 'local'.
publishMavenAsciidoctorJApiPublicationToMavenLocal - Publishes Maven publication 'mavenAsciidoctorJApi' to the local Maven repository.
publishMavenAsciidoctorJApiPublicationToSonatypeRepository - Publishes Maven publication 'mavenAsciidoctorJApi' to Maven repository 'sonatype'.
publishMavenAsciidoctorJArquillianExtensionPublicationToLocalRepository - Publishes Maven publication 'mavenAsciidoctorJArquillianExtension' to Maven repository 'local'.
publishMavenAsciidoctorJArquillianExtensionPublicationToMavenLocal - Publishes Maven publication 'mavenAsciidoctorJArquillianExtension' to the local Maven repository.
publishMavenAsciidoctorJArquillianExtensionPublicationToSonatypeRepository - Publishes Maven publication 'mavenAsciidoctorJArquillianExtension' to Maven repository 'sonatype'.
publishMavenAsciidoctorJPublicationToLocalRepository - Publishes Maven publication 'mavenAsciidoctorJ' to Maven repository 'local'.
publishMavenAsciidoctorJPublicationToMavenLocal - Publishes Maven publication 'mavenAsciidoctorJ' to the local Maven repository.
publishMavenAsciidoctorJPublicationToSonatypeRepository - Publishes Maven publication 'mavenAsciidoctorJ' to Maven repository 'sonatype'.
publishMavenAsciidoctorJTestSupportPublicationToLocalRepository - Publishes Maven publication 'mavenAsciidoctorJTestSupport' to Maven repository 'local'.
publishMavenAsciidoctorJTestSupportPublicationToMavenLocal - Publishes Maven publication 'mavenAsciidoctorJTestSupport' to the local Maven repository.
publishMavenAsciidoctorJTestSupportPublicationToSonatypeRepository - Publishes Maven publication 'mavenAsciidoctorJTestSupport' to Maven repository 'sonatype'.

Running ./gradlew publishAllPublicationsToLocalRepository seems to also publish the cli module, but to avoid any conflicts it would be great to give it a distinct name.

test more_than_one_input_file_should_throw_an_exception does not make sense. Maybe the name is wrong?

Yes, that test can get a better name.

Seems to me only AsciidoctorInvoker should be in package org.asciidoctor.jruby.cli, the rest don't depend on jRuby implementation and could be moved to org.asciidoctor.cli?

Yes, that makes sense.

abelsromero commented 1 year ago

Updated publication name.
Updated test name more_than_one_input_file_should_throw_an_exception -> should_convert_multiple_inputs

Seems to me only AsciidoctorInvoker should be in package org.asciidoctor.jruby.cli, the rest don't depend on jRuby implementation and could be moved to org.asciidoctor.cli?

Yes, that makes sense.

It feels good when we have free hand for breaking changes :dancers:

In the final structure, which imo makes more sense, we have the general package org.asciidoctor.cli with shared elements, and org.asciidoctor.cli.jruby with the jruby parts.

We could have an AsciidoctorInvoker interface but I'd rather go with KIS until we have more implementations available.

abelsromero commented 1 year ago

If it's ok, can I push "merge" myself? :pleading_face:

robertpanzer commented 1 year ago

Done😊

asciidoctor / asciidoctorj