Comments welcome #2140
Comments welcome #2140
Conversation
…appropriate instead of scalar(x->unlist)
…d splitting xml text nodes with comments; remove obsolete form of DefLigature
| my $prev; | ||
| while (($prev = $node->previousSibling) && ($prev->nodeType != XML_ELEMENT_NODE)) { | ||
| $node = $prev; } | ||
| return $prev; } |
There was a problem hiding this comment.
Naming question: Given nextSibling and previousSibling have the naming scheme with the modifier in front and the kind of object second, I'd be tempted to suggest next_element and previous_element (which are in a way abbreviated forms of get_next_element and get_previous_element).
That's roughly how I've internalized the already existing element_nodes which seems to align with childNodes in the libxml wrapper.
There was a problem hiding this comment.
I guess at the time I was writing, I was emphasizing "element".
| return 0 unless IsEmpty($thing->unlist); } | ||
| elsif ($ref eq 'LaTeXML::Core::Token') { | ||
| my $cc = $$thing[1]; | ||
| return 0 if ($cc == CC_LETTER) || ($cc == CC_OTHER) || ($cc == CC_ACTIVE) || ($cc == CC_CS); } |
There was a problem hiding this comment.
Hm, this seems incomplete? A T_BEGIN, T_END, T_MATH, T_ALIGN, T_PARAM aren't empty either. Maybe it's somewhat briefer to invert the condition:
return 0 unless ($cc == CC_COMMENT) || ($cc == CC_MARKER) || ($cc == CC_IGNORE);There was a problem hiding this comment.
though if this is meant to be used for a single Token, I'd also wonder if we shouldn't treat all tokens that have empty text as empty as well.
return 0 unless ($cc == CC_COMMENT) || ($cc == CC_MARKER) || ($cc == CC_IGNORE)
|| (!length($$thing[0]));Edit: Or indeed use the regex leveraged a little lower for non-space $$thing[0] !~ /^\s*$/
There was a problem hiding this comment.
I think you have the logic flipped; this line should short-circuit immediately returning "false" if we encounter a token that is not empty.
OTOH, your point is good: should we consider T_LETTER("") to be empty?
OTOOH, I expect this issue may have to be revisited to distinguish between truly empty, and just spacey things? Hopefully I can defer that concern?
|
You raise good questions, but with the refactors in this PR, it should be easy to find & revisit later if we want to. |
This PR deals more consistently with comments, whether Tokens, digested or XML nodes, appearing in places we previously didn't expect them.
The function
IsEmptytests whether a tokens or list is empty, ignoring comments, and is generally preferred overscalar($x->unlist );element_next($node), element_prev($node)are usually preferred over$node->previousSibling. Also, avoid splitting xml text nodes with comments, pushing them to the front.Also, remove an obsolete form of
DefMathLigature.