asciidoctor / asciidoctorj

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

Fix Macro APIs to return Structural or Phrase Nodes instead of java.l… #1084

Closed robertpanzer closed 2 years ago

robertpanzer commented 2 years ago

…ang.Object.

Kind of change

Description

What is the goal of this pull request?

As @melix had to experience painfully this week the macro APIs are not really showing what type of node an extension has to return. Currently both InlineMacroProcessors and BlockMacroProcessors define java.lang.Object as the return type. Instead of parameterizing the return type of the method I parameterized the parameter for the parent node 🤦 I don't know what I had drunk when looking over that.

This PR tries to update the signatures so that it's clearer for the user what type of nodes they have to return.

How does it achieve that?

It updates the InlineMacroProcessor.process() to return a PhraseNode instead of java.lang.Object and BlockMacroProcessor.process() to return StructuralNode.

@mojavelinux Is it even possible that anything else than a StructuralNode/AbstractBlock is passed as a parent to an Inline- or BlockMacroProcessor? We currently have the topmost base class ContentNode as the type of the parent parameter. If not I could remove that type parameter.

Are there any alternative ways to implement this?

I don't think so.

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

This is a breaking change. The source code of existing extensions must be updated.

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

robertpanzer commented 2 years ago

If I read the code correctly in substitutors.rb the parent passed to an InlineMacro extension should always be a Block or a Section, that is a StructuralNode, instead of the more generic ContentNodes. Therefore I changed the signature correspondingly and both Block- and InlineMacros now receive StructuralNodes as the first parameter. Also added a description about required changes to the migration guide.

mojavelinux commented 2 years ago

If I read the code correctly in substitutors.rb the parent passed to an InlineMacro extension should always be a Block or a Section, that is a StructuralNode

That's correct.