asciidoctor / asciidoctorj

:coffee: Java bindings for Asciidoctor. Asciidoctor on the JVM!
http://asciidoctor.org
Apache License 2.0
623 stars 173 forks source link

Remove AsciidoctorUtils class #1170

Closed abelsromero closed 1 year ago

abelsromero commented 1 year ago

Kind of change

Description

Remove AsciidoctorUtils class. What is the goal of this pull request?

This class contained a method to convert options into asciidoctor cli. Used only for logging and tests it's considered not useful.

Other changes:

How does it achieve that?

Removes class and extracts a single method that is still used once as private method in the calling class (JRubyAsciidoctor).

Are there any alternative ways to implement this?

If there's any concern about users impacted. We could keep the class and remove its usage, but it's already in an internal package and we can. It should not be a surprise.

Are there any implications of this pull request? Anything a user must know?

Only if they are using the class.

Issue

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

Fixes #1169

Release notes

Please add a corresponding entry to the file CHANGELOG.adoc

abelsromero commented 1 year ago

I like those, removing is good image

abelsromero commented 1 year ago

I had a look at the "test upstream" and I don't understand what it's doing. It installs a local jar containing the asciidoctor gem? and then the ruby gradle plugin uses it instead of the gem?

mojavelinux commented 1 year ago

Yes, the test upstrea is testing against the unreleased version of Asciidoctor. Since it's not distributed, it must be installed locally and used as though it had been retrieved from a repository.

abelsromero commented 1 year ago

Yes, the test upstrea is testing against the unreleased version of Asciidoctor. Since it's not distributed, it must be installed locally and used as though it had been retrieved from a repository.

Thanks! My doubt is more about the "how" though. In a normal gradle build com.github.jruby-gradle.base downloads the gem directly into ./build/gems, but seeing that we install the gem in test upstream with the maven gem-maven-plugin, that means that the gem dependency we define can also process local jars. Not being able to generate the same jar now, makes it's hard to know what the plugin expects (yes, I haven't read their docs fully yet :sweat_smile: ).

I'll do some testing today, maybe we can explore another option. I just saw we don't pull transitive dependencies for asciidoctor which was one of my concerns...I wonder if we can just unpack the asciidoctor zip from gh in ./build/gems.

mojavelinux commented 1 year ago

I'm testing my memory, but I think the how predated the switch to Gradle. So it's very possible that it is an outdated approach and worth rethinking. But without looking into it deeper, I wouldn't be able to say more just from memory alone.

robertpanzer commented 1 year ago

Nice, loosing some weight :)