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

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

Merged
merged 3 commits into from
May 6, 2020

Conversation

GarkGarcia
Copy link
Collaborator

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
Copy link
Collaborator Author

@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
Copy link
Member

pepijnve commented May 6, 2020

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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.

@GarkGarcia GarkGarcia merged commit 5a1e5ca into asciidoctor:obj_ast May 6, 2020
@GarkGarcia GarkGarcia deleted the obj_ast branch May 8, 2020 12:16
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 this pull request may close these issues.

2 participants