-
Notifications
You must be signed in to change notification settings - Fork 172
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
Reuse Asciidoctor Ruby invoker #1249
Conversation
I can have a look tomorrow. But 1 question, from the description it seems this is preferable to #1248, is there something between the two you are not convinced, so both should be checked. Or this is the final approach? To me it makes sense to reuse the invoker, but I haven't looked into the details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some comments but no major concerns, only the fact that it seems this does not support input redirection and duplicates some code?
asciidoctorj-cli/src/main/java/org/asciidoctor/cli/AsciidoctorCliOptions.java
Outdated
Show resolved
Hide resolved
asciidoctorj-cli/src/main/java/org/asciidoctor/cli/AsciidoctorCliOptions.java
Show resolved
Hide resolved
asciidoctorj-cli/src/main/java/org/asciidoctor/cli/AsciidoctorCliOptions.java
Outdated
Show resolved
Hide resolved
asciidoctorj-core/src/main/java/org/asciidoctor/jruby/internal/RubyGemsPreloader.java
Outdated
Show resolved
Hide resolved
asciidoctorj-cli/src/main/java/org/asciidoctor/cli/jruby/AsciidoctorInvoker.java
Show resolved
Hide resolved
asciidoctorj-cli/src/main/java/org/asciidoctor/cli/jruby/AsciidoctorInvoker.java
Outdated
Show resolved
Hide resolved
asciidoctorj-cli/src/main/java/org/asciidoctor/cli/jruby/AsciidoctorInvoker.java
Show resolved
Hide resolved
I think this PR addresses #193. Although this implementation takes a different approach, the spirit of the solution is still the same, to reuse the Asciidoctor CLI instead of maintaining a clone of it. In that regard, I think it's a great step forward. |
@abelsromero About
I tried this, and I see the output piped to less:
Can you share some details about what failed for you? |
…e AsciidoctorJ API. Added more comments, minor code cleanup.
I mentioned because I saw the code being remove, but it's working fine for me (Linux). I built a distribution and converted both files and from cli |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, I am doing suggestions based on my biases, no need to follow all of them. I am happy with the result because we are now reducing code
but you @robertpanzer have the ultimate word.
👍 |
Thank you for opening a pull request and contributing to AsciidoctorJ!
Please take a bit of time giving some details about your pull request:
Kind of change
Description
What is the goal of this pull request?
This PR should do the same thing as #1248, that is better align the behavior of the AsciidoctorJ CLI with the behavior of the Asciidoctor Ruby CLI.
This PR however, reuses the original Asciidoctor::Cli::Invoker and feeds its own options into it instead of letting Asciidoctor Ruby parse them.
So chances are that this works a bit better than the other PR.
The code is still raw, but I'd like to get some feedback on what approach to pursue.
How does it achieve that?
Are there any alternative ways to implement this?
Are there any implications of this pull request? Anything a user must know?
Issue
If this PR fixes an open issue, please add a line of the form:
Fixes #Issue
Release notes
Please add a corresponding entry to the file CHANGELOG.adoc