Closed abelsromero closed 1 year ago
Thanks for starting the work on enhancing the catalogAssets feature!
we cannot call convert because it fails (see https://github.com/asciidoctor/asciidoctorj/issues/1166#issuecomment-1500921789).
Can you shed some light on what is failing?
Asciidoctor::convert
documents that it returns a Document when converting to a file, and the string otherwise:
I see that we're not returning the Document object when converting to a file, just returning null instead. Is that what you mean with "it fails"?
Is that what you mean with "it fails"?
Yes, haven't had time to check yet. I assume there's some error in the bindings that should not be very complicated :crossed_fingers:
I see we explicitly check the return type and just insert null https://github.com/asciidoctor/asciidoctorj/blob/0240f02d54268bc484d6da210d0803c0156578e0/asciidoctorj-core/src/main/java/org/asciidoctor/jruby/internal/JRubyAsciidoctor.java#L310. And going back into the git history I could not find a real reason for it, seems to come from initial versions and it has been preserved. Could be that we considered returning Document odd when the Java AST was not well defined. I think the best is to align with Asciidoctor Ruby and we should not take actions that limit functionality. Unless we want to create new methods to distinguish "conversion" from "conversion + return AST", but given the combinations of methods and options are already not simple (eg. convert + toFile == convert_file), I would not add more layers.
So I changed the check that returned null to actually return Document, and furthermore seeing the code now I see why convert
returns the dynamic Interface seeing in the comment instead of a proper String. I'll open a separate issue for that.
I can't recall if this has always been the case, or whether I changed it to this to avoid the problems with the wrappers generated by JRuby. At least for AsciidoctorJ 3.0 we could check if the expected class is an AST class, and in that case try to apply the NodeConverter.
At least for AsciidoctorJ 3.0 we could check if the expected class is an AST class, and in that case try to apply the NodeConverter.
That's my idea, adaptReturn takes care of it for now in a simple way. In another PR we can improve and create some fancy Java class ReturnTypeAdapterVisitorPattern. But for now, it gets the job done.
I just updated the changelog, nothing else to add or change from me.
Unless objected, this being a new feature I am not considering porting to v2.5.x
anything else to improve? 👀
Sorry for being behind a bit, I'll check it over the weekend.
Thanks! Looks good to me.
Thanks!
Kind of change
Description
What is the goal of this pull request? DRAFT PR, WIP
Offer users the option to obtain images from the assets catalog. However, it only works when calling
load()+getContent()
, and unlike in Asciidoctor Ruby, we cannot callconvert
because it fails (see comment).How does it achieve that?
Exposes the images properties from the catalog. However, this PR is not done yet untill:
convert
to obtain theDocument
instanceWhenReadingImagesFromCatalogAssetFromConverter
) :point_right: I think that fixing the first issue should fix this.Are there any alternative ways to implement this?
WIP
Are there any implications of this pull request? Anything a user must know?
Not
Issue
If this PR fixes an open issue, please add a line of the form:
Fixes #1166
Release notes
Please add a corresponding entry to the file CHANGELOG.adoc