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

Failure getting blocks when using description lists #1082

Closed
someth2say opened this issue Feb 8, 2022 · 8 comments · Fixed by #1083
Closed

Failure getting blocks when using description lists #1082

someth2say opened this issue Feb 8, 2022 · 8 comments · Fixed by #1083

Comments

@someth2say
Copy link

See https://asciidoctor.zulipchat.com/#narrow/stream/279644-users.2Fasciidoctorj/topic/Failure.20getting.20.60blocks.60.20when.20using.20description.20lists

Seems that DescriptionListImpl.blocks returns a RubyArrayTwoObject, instead of a List<StructuralNode>.
This makes a bunch of processing steps more complicated and error-prone.
For example, this Treeprocessor:

   public static class SolutionsProcedureTreeprocessor extends Treeprocessor {
        public SolutionsProcedureTreeprocessor() {}
        @Override
        public Document process(Document document) {
            processBlock((StructuralNode) document, 0);
            return document;
        }

        private void processBlock(StructuralNode block, int deep) {
            List<StructuralNode> blocks = block.getBlocks();
            for (int i = 0; i < blocks.size(); i++) {
                final StructuralNode currentBlock = blocks.get(i);
                if (currentBlock instanceof StructuralNode) {
                    System.out.println(" ".repeat(deep) + currentBlock.getClass());
                    processBlock(currentBlock, ++deep);
                }
            }
        }
    }

Fails when trying to invoke block.getBlocks() because the block is not a StructuralNode, but a RubyArrayTwoObject:

org.asciidoctor.jruby.internal.AsciidoctorCoreException: org.jruby.exceptions.NoMethodError: (NoMethodError) asciidoctor: FAILED: nav.adoc: Failed to load AsciiDoc document - undefined method `blocks' for #<Array:0xfb0dc3ba>

Proposal: Overwrite the DescriptionListImpl.blocks method so it returns a flattened list of nodes.

@abelsromero
Copy link
Member

We should think this through and maybe we don't want to follow same approach as in Ruby core. I personally have doubts of the current structure, for instance why the terms is a list.

@mojavelinux
Copy link
Member

I agree that we don't need to follow the structure used by Asciidoctor Ruby. The structure it uses was selected quickly in the early days of Asciidoctor and never revisited. It piggy backed off of the existing List / ListItem types and is very low-level for the purpose of speed and efficiency. But that doesn't make it right. I've always felt that we needed to remodel that block type eventually.

@robertpanzer
Copy link
Member

robertpanzer commented Feb 9, 2022

I just compared to Asciidoctor.js and it seems like it returns the list items on DescriptionList.getBlocks();
However, if I didn't do anything wrong, the ListItems are not Blocks so it results in a similar problem one level deeper. It's actually the same problem.

I'll see if I can fix it in a similar way to Asciidoctor.js.

for instance why the terms is a list.

What is a better way to model a list item with multiple terms?

@robertpanzer
Copy link
Member

I vaguely remembered that we had already touched this in the past and found this :)
#408 (comment)

@robertpanzer
Copy link
Member

Since a DescriptionListEntry, the item, is not a StructuralNode it's probably best to return an empty list.

@abelsromero
Copy link
Member

Since a DescriptionListEntry, the item, is not a StructuralNode it's probably best to return an empty list.

How would a user traverse the whole tree then? I see no generic "getChildren()"

@robertpanzer
Copy link
Member

Would it help if we made all interfaces generic and added a getChildren() returning the blocks or the items?
I am unsure if this is really ergonomic:

public interface StructuralNode<T> {
  public List<T> getChildren();
}
public interface Block extends StructuralNode<StructuralNode> {}
public interface DescriptionList extends StructuralNode<DescriptionListEntry> {}

I am unsure if this is sth anybody would like to program against, given that StructuralNode and DescriptionListEntry have no commonalities (except what java.lang.Object has).

@abelsromero
Copy link
Member

I am aware it's not trully an AST due to how things are parsed and I don't have enough context yet, but being able to traverse the whole data seems a requirement. Maybe we can find a compromise here and start discussing future changes for v3.0.0?

robertpanzer added a commit to robertpanzer/asciidoctorj that referenced this issue Feb 13, 2022
…s() instead of return a list of DescriptionListItems, which are not blocks
robertpanzer added a commit that referenced this issue Feb 15, 2022
Fixes #1082. Return empty list in DescriptionList.getBlocks() instead…
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 a pull request may close this issue.

4 participants