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

Remove AsciidoctorUtils class #1170

Conversation

abelsromero
Copy link
Member

Kind of change

  • Bug fix
  • New non-breaking feature
  • New breaking feature
  • Documentation update
  • Build improvement

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:

  • Add AssertJ dependency to Java projects by default
  • Refactored AsciidoctorUtils.isOptionWithAttribute into JRubyAsciidoctor.containsAttributeWithValue

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
Copy link
Member Author

abelsromero commented Apr 7, 2023

I like those, removing is good
image

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

Other changes:
* Add AssertJ dependency to Java projects by default
* Refactored AsciidoctorUtils.isOptionWithAttribute into JRubyAsciidoctor.containsAttributeWithValue

Fixes asciidoctor#1169
@abelsromero
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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 😅 ).

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
Copy link
Member

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
Copy link
Member

Nice, loosing some weight :)

@robertpanzer robertpanzer merged commit 0240f02 into asciidoctor:main Apr 9, 2023
@abelsromero abelsromero deleted the issue-1169-remove-AsciidoctorUtils-class branch April 21, 2023 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove AsciidoctorUtils class
3 participants