move \trim from parser to renderer

[MrSpoocy]

Hello,

I am currently using your libary and have written several extensions. The possibility to make changes via EventDispatcher is very good for me. However, I noticed something that prevents me from writing an extension for the event "DocumentParsedEvent". The fact that at this time the inline text was already processed via trim. There is no way for me to find out if the original text has "||" or "| |" written in a table.

For me it is more logical, if the parser does not trim the text yet and only the render takes over the trim.

This change would possibly mean breaking changes for extensions, but would offer more possibilities for the future. As a test I made this change and ran the test once. Only 15 11 8 4 tests failed.

In my case I want to extend the table and convert "||" to colspan and |^ to rowspan without rewriting the whole TableExtension.

[colinodell]

Thanks for your kind feedback!

The GFM spec is not explicit about when the spaces should be trimmed, just that they are:

Each row consists of cells containing arbitrary text, in which inlines are parsed, separated by pipes (|). A leading and trailing pipe is also recommended for clarity of reading, and if there’s otherwise parsing ambiguity. Spaces between pipes and cell content are trimmed. Block-level elements cannot be inserted in a table.

But given that the two preceding sentences are discussing parsing, and that Github's implementation performs trimming during parsing, I do think it's probably most appropriate to keep trim() within the parsing step.

In my case I want to extend the table and convert "||" to colspan and |^ to rowspan without rewriting the whole TableExtension.

I can understand how your proposed change would allow you to do that. But I do think that converting Markdown into certain types of elements should be done during parsing, since that's the only time you're guaranteed to have the full information about the syntax that was used. DocumentParsedEvent should ideally only be used to manipulate elements using just the information in the AST and not the original syntax it was parsed from.

I would be open to refactoring the TableParser so it's easier for you to extend. I haven't given much thought into how to best do that, so any ideas you have would be very helpful! And if we can do that without breaking backward compatibility we could introduce those changes in a new minor version.

[MrSpoocy]

Moving the trim from the parser to the renderer was more a general idea. Which had been shown by my concrete case. I followed the other extensions (Attributes, Embed, Footnote) which also all rely on the fact that the Markdown has already been processed. Because then it is also ensured for the extension that e.g. we are dealing with a URL without having to perform the complex RegEx check again.

In my test at the moment, it is so, that the relocation only leads to further nodes, if directly before or after an inline-none-text-node comes. Then an empty text node is created.

At the moment I could solve it without a breaking change. I have extended the class HtmlElement by one parameter.

EDIT:

In the remaining 4 failed test, 3 are MarkdownToXML.

[MrSpoocy]

I've created a pull-request #924, so you can look over the changes to get a better impression of my idea.

[colinodell]

After giving this more thought I still feel like the trim() behavior should be kept as-is because it most closely aligns with the CommonMark and GFM specs.

There is no way for me to find out if the original text has "||" or "| |" written in a table ... In my case I want to extend the table and convert "||" to colspan and |^ to rowspan

A colspan or rowspan would be an HTML attribute applied to the TableCell node. Because TableCell is a block and not an inline, the || vs | | syntax should ideally be detected during block parsing (which happens well before any trimming or inline parsing). Using inline parsing to detect this and update the block's attributes sounds like more of a workaround (or an X-Y problem).

So I do still think that changing the trim() behavior is not the right approach here.

What if we instead removed final from TableParser and changed private function parseCell to be protected? That would allow you to extend TableParser and know the original cell contents (which might be a single space) at the time the TableCell is parsed. Would that solution work for your case?

[MrSpoocy]

I am also for the fact that already with the block parsing it should be recognized whether || or | | was written. But unfortunately the class is final. And I think it's a bad idea to remove it.

I also don't abuse inline parsing to detect this, it's more like checking for the absence of inline. I put the code online so you can get an idea (https://github.com/MrSpoocy/commonmark/tree/extended_table/src/Extension/ExtendedTable).

Of course I am also open to extend your current TableExtension with the possibility of colspan/rowspan. I was just not sure if this is desired. That's why I looked for another way.

[colinodell]

But unfortunately the class is final. And I think it's a bad idea to remove it.

Yeah, there's a lot of value to using final and I'd prefer to keep it wherever we can, but I'm also okay making this exception. It would certainly be easier than refactoring the entire parser to be more flexible, which we could always do in a future version anyway :)