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

ListItem doesn't quack like an AbstractNode #2979

Open
mattwynne opened this Issue Nov 30, 2018 · 3 comments

Comments

Projects
None yet
2 participants
@mattwynne

mattwynne commented Nov 30, 2018

I'm writing a TreeProcessor extension and have had to put in some defensive stuff to handle ListItem nodes (for a definition list) which do not seem to have #role or #blocks methods as I would have expected [from the docs].

The error I see is undefined method 'has_role?' for #<Array:0x007f82da398a50> (NoMethodError) but the node is definitely an Asciidoctor::ListItem.

This is asciidoctor 1.5.8

@mojavelinux

This comment has been minimized.

Member

mojavelinux commented Dec 1, 2018

The data structure for the items of a description list block differs from other lists. The items property uses the following structure:

[[term, term, ...], description]

Each term and the description are ListItem objects.

You can see how these are iterated in the default HTML converter.

node.items.each do |terms, dd|
[*terms].each do |dt|
result << %(<dt#{dt_style_attribute}>#{dt.text}</dt>)
end
if dd
result << '<dd>'
result << %(<p>#{dd.text}</p>) if dd.text?
result << dd.content if dd.blocks?
result << '</dd>'
end
end

The node API was not originally designed to be public, so it has some quirks that carry over from the early days. We could entertain a cleaner API after the 2.0.0 release.

@mojavelinux mojavelinux added this to the support milestone Dec 1, 2018

@mojavelinux mojavelinux self-assigned this Dec 1, 2018

@mattwynne

This comment has been minimized.

mattwynne commented Dec 1, 2018

Thanks for the quick reply and deep explanation Dan.

I think an API for walking the tree that just yielded nodes and allowed, or even expected, you to completely replace the children would be ideal for processing the AST. It would be nice to be able to use #reduce on the blocks instead of fiddling around mutating the existing Array in-place.

@mojavelinux

This comment has been minimized.

Member

mojavelinux commented Dec 1, 2018

But that would be true for all block nodes. Currently, the blocks (aka items for lists) cannot be assigned. So while it's a good point, it doesn't really pertain to this case in particular.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment