search

asciidoctor / asciimath

Asciimath parser
MIT License
24 stars 16 forks source link

Customizable renderer tables #47

Closed GarkGarcia closed 4 years ago

GarkGarcia commented 4 years ago

This is a follow up to #46.

Made the LaTeX render internal symbols table public and customizable. Also renamed LatexBuilder::SYMBOLS_TABLE and MarkupBuilder::DEFAULT_DISPLAY_SYMBOL_TABLE to _::SYMBOL_TABLE.

pepijnve commented 4 years ago

Why the rename? The DEFAULT bit communicates that this is not the only possible one, just the default. DISPLAY vs PARSER communicates that they're two different things. Contrasts with Parser::DEFAULT_PARSER_SYMBOL_TABLE and Parser::DEFAULT_PARSER_COLOR_TABLE

GarkGarcia commented 4 years ago

Why the rename? The DEFAULT bit communicates that this is not the only possible one, just the default. DISPLAY vs PARSER communicates that they're two different things. Contrasts with Parser::DEFAULT_PARSER_SYMBOL_TABLE and Parser::DEFAULT_PARSER_COLOR_TABLE

Adding the DEFAULT bit back makes sense to me. It's a bit a verbose name, but it indeed helps communicate the intent of the constant.

However, I don't see a need for the DISPLAY/PARSER. There isn't a Parser::DEFAULT_DISPLAY_SYMBOL_TABLE or a MarkupBuilder::DEFAULT_PARSER_SYMBOL_TABLE that requires disambiguation. The intent of Parser::DEFAULT_SYMBOL_TABLE and MarkupBuilder::DEFAULT_SYMBOL_TABLE is already clear.

pepijnve commented 4 years ago

I'm a Java developer in the day. We like long names 😄 I would be careful with giving these things the same name though. They're both just Hashes and if they have the same implied type (i.e., SYMBOL_TABLE) it really make it look like they're structurally the same which they're not. Maybe we need a better name than SYMBOL_TABLE.

GarkGarcia commented 4 years ago

They're both just Hashes and if they have the same implied type (i.e., SYMBOL_TABLE) it really make it look like they're structurally the same which they're not.

I see. Could you elaborate a bit on what makes them distinct. I agree that if the are not structurally the same we should avoid calling them the same thing, but I think MarkupBuilder::DEFAULT_DISPLAY_SYMBOL_TABLE and LatexBuilder::SYMBOLS_TABLE should be structurally equivalent, for consistency sake.

pepijnve commented 4 years ago

Could you elaborate a bit on what makes them distinct.

One controls tokenisation of the parser and contains case sensitive Strings as keys. The parser tries to find a longest matching string in this table. The other controls output and contains symbols as keys that are produced by the parser. So the output of the parser table serves as the input of the display table.

I wouldn't worry too much about keeping Markup and LaTeX the same. Agreed you want to go for some degree of consistency, but there's no need (and it's not desirable) to introduce coupling between the two implementations. The needs of the two may be similar or identical today , but that's not something intrinsic.

GarkGarcia commented 4 years ago

Maybe we need a better name than SYMBOL_TABLE.

We probably do. It looks like Parser::DEFAULT_PARSER_SYMBOL_TABLE and MarkupBuilder::DEFAULT_DISPLAY_SYMBOL_TABLE/LatexBuilder::SYMBOLS_TABLE are fundamentally different in some sense.

GarkGarcia commented 4 years ago

One controls tokenisation of the parser and contains case sensitive Strings as keys. The parser tries to find a longest matching string in this table. The other controls output and contains symbols as keys that are produced by the parser. So the output of the parser table serves as the input of the display table.

I see. What if we were to rename Parser::DEFAULT_PARSER_SYMBOL_TABLE to Parser::DEFAULT_TOKENAZATION_TABLE and MarkupBuilder::DEFAULT_DISPLAY_SYMBOL_TABLE/LatexBuilder::SYMBOLS to Whatever::DEFAULT_TRANSLATION_TABLE?

GarkGarcia commented 4 years ago

One controls tokenisation of the parser and contains case sensitive Strings as keys. The parser tries to find a longest matching string in this table. The other controls output and contains symbols as keys that are produced by the parser. So the output of the parser table serves as the input of the display table.

I see. What if we were to rename Parser::DEFAULT_PARSER_SYMBOL_TABLE to Parser::DEFAULT_TOKENAZATION_TABLE and MarkupBuilder::DEFAULT_DISPLAY_SYMBOL_TABLE/LatexBuilder::SYMBOLS to Whatever::DEFAULT_TRANSLATION_TABLE?

They could still rely on SymbolTableBuilder internally.

GarkGarcia commented 4 years ago

I wouldn't worry too much about keeping Markup and LaTeX the same. Agreed you want to go for some degree of consistency, but there's no need (and it's not desirable) to introduce coupling between the two implementations. The needs of the two may be similar or identical today , but that's not something intrinsic.

Fair enough. I can see how too much coupling could become a problem.

GarkGarcia commented 4 years ago

@pepijnve I tried adding some documentation about the current state of things in regards to extensions and renderer customization. Could you take a look at the last commit?

GarkGarcia commented 4 years ago

@pepijnve Would you mind if we where to rename AsciiMath::Tokenizer::DEFAULT_PARSER_SYMBOLS_TABLE to AsciiMath::Tokenizer::DEFAULT_TOKENS_TABLE? I believe this would make the intent of the constant clearer. Also, this would allow us to drop the DISPLAY bit in AsciiMath::MarkupBuilder::DEFAULT_DISPLAY_SYMBOLS_TABLE.

GarkGarcia commented 4 years ago

@pepijnve The renaming of the constants was stalling this PR. Because of that I've reverted back to the old naming conventions (although I renamed LatexBuilder::SYMBOLS to LatexBuilder::DEFAULT_DISPLAY_SYMBOL_TABLE for consistency). Are there any objections of yours to the current proposal?

pepijnve commented 4 years ago

The renaming of the constants was stalling this PR.

@GarkGarcia sorry to give you that impression. I'm swamped with other work at the moment so I just haven't had time to make progress on this. I'll try to find some time to review this soon.