search

asciidoctor / asciimath

Asciimath parser
MIT License
24 stars 16 forks source link

Adapt the LaTeX builder to the new, classe-based AST #33

Closed GarkGarcia closed 4 years ago

GarkGarcia commented 4 years ago

A follow up to #30.

There's still a spec which is not passing. The error has to do with how the first argument of color is parsed.

GarkGarcia commented 4 years ago

@pepijnve Are you sure the expressing color(red)(x) should be parsed as binary(symbol('color'), grseq('r', 'e', 'd'), group('x'))?

Wouldn't something like binary(symbol('color'), text('red'), group('x')) be more appropriate?

pepijnve commented 4 years ago

Purely at the grammar level it's correct. color x y matches the binary symbol rule and gets parsed accordingly. The tokeniser matches the longest known symbol it can, text, numbers or single character identifiers. As a consequence, since red is not a known symbol, you get the sequence of identifiers r, e, d.

The consequence of this is that if we want to match red instead (and colors in general) we need to either add all known colors as symbols or add special case parsing and tokenisation logic.

The alternative (and that's what I've implemented) is to move this 'interpretation' step of the AST down to the renderers. The logic is located at https://github.com/asciidoctor/asciimath/blob/obj_ast/lib/asciimath/markup.rb#L462.

This isn't set in stone btw. We can pull that method back up to the parser for instance and pass a Text node downstream. Maybe that makes the most sense since every renderer would have to duplicate this logic otherwise.

GarkGarcia commented 4 years ago

The alternative (and that's what I've implemented) is to move this 'interpretation' step of the AST down to the renderers. The logic is located at https://github.com/asciidoctor/asciimath/blob/obj_ast/lib/asciimath/markup.rb#L462.

Makes sense. This makes the rendering of color x y more flexible. Just fixed the issue 😁️

GarkGarcia commented 4 years ago

This isn't set in stone btw. We can pull that method back up to the parser for instance and pass a Text node downstream. Maybe that makes the most sense since every renderer would have to duplicate this logic otherwise.

We should probably create a "standard" for color descriptors. If we did so, we would be able to enforce the validity of color descriptors at the parser.

GarkGarcia commented 4 years ago

This isn't set in stone btw. We can pull that method back up to the parser for instance and pass a Text node downstream. Maybe that makes the most sense since every renderer would have to duplicate this logic otherwise.

We should probably create a "standard" for color descriptors. If we did so, we would be able to enforce the validity of color descriptors at the parser.

I'll create an issue to discuss this. Meanwhile, I'll merge this PR.