Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract CLI Java components into new submodule #1149

Merged

Conversation

abelsromero
Copy link
Member

Kind of change

  • Bug fix
  • 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 abelsromero force-pushed the issue-246-extract-cli-into-submodule branch 3 times, most recently from 520e506 to 80b18bb Compare March 21, 2023 22:54
@abelsromero
Copy link
Member Author

A few extra questions...

  1. Should ext.publicationName have some specifc value? I don't think it's necessary but Gradle complains if it's missing.
  2. test more_than_one_input_file_should_throw_an_exception does not make sense. Maybe the name is wrong?
  3. 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
Copy link
Member

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 abelsromero force-pushed the issue-246-extract-cli-into-submodule branch from 80b18bb to 648e0e7 Compare March 23, 2023 19:46
* 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.

Fixes asciidoctor#246
@abelsromero
Copy link
Member Author

  • 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 👯

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.

image

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

@abelsromero
Copy link
Member Author

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

@robertpanzer robertpanzer merged commit 44b5776 into asciidoctor:main Mar 23, 2023
@robertpanzer
Copy link
Member

Done😊

@abelsromero abelsromero deleted the issue-246-extract-cli-into-submodule branch April 21, 2023 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move CLI to its own subproject
2 participants