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

Respect node settings when rendering #10

Closed
5 tasks done
GuySartorelli opened this issue Oct 31, 2023 · 3 comments
Closed
5 tasks done

Respect node settings when rendering #10

GuySartorelli opened this issue Oct 31, 2023 · 3 comments
Labels
bug Something isn't working enhancement New feature or request help wanted Extra attention is needed

Comments

@GuySartorelli
Copy link
Contributor

GuySartorelli commented Oct 31, 2023

As discussed in #9, various nodes have additional data explaining how they are intended to be rendered. For example, the fenced code node tells you its offset (indent), what delimiter character to use, and how many of that delimiter character should be used.

In some cases respecting this data will be useful only in that you're more likely to match the output to the given input - but in other cases ignoring that data can result in invalid (or at least incorrect) markdown. There's some examples of that in the above linked PR.

I've identified the following as not using the properties available when rendering nodes:

@stefanzweifel
Copy link
Owner

This issue seems to be more complex than I thought. 😐

In #11 I've fixed StrongRenderer and EmphasisRenderer to use getOpeningDelimiter() and getClosingDelimiter().
(For v2 I will probably drop the use_asterisk and use_underscore config, if that's not something that's comming from commonmark itself; didn't check)

Ignoring type in HtmlBlockRenderer seems fine to me, as it doesn't seem it should make any impact, on how the node is rendered. (Feel free to drop any examples that teach me otherwise)

IndentedCodeRenderer needs redoing. Looking at the code, I now remember that I really struggled to find exact examples of what each renderer should look like, when I developed this package.

ListBlockRenderer and ListItemRenderer: These definitely need an overhaul. In #7 I've fixed an issue with multi-line list items, with a solution that always felt like a cheap fix.

I need some more head-space to tackle this. And the package definitely needs a better test-suite. The current tests have all been inspired by the league/commonmark suite, but it always felt a bit too unit-testy for me.
Will update the kitchen-sink.md with a bit more examples to make testing a bit more straight forward.

@GuySartorelli
Copy link
Contributor Author

GuySartorelli commented Nov 14, 2023

Regarding the HTML block, here's the commonmark spec: https://spec.commonmark.org/0.30/#html-blocks
It lists 7 different ways a HMTL Block can be declared.

I haven't tested to see if, for example, <![CDATA[ is included in the value you get from $node->getLiteral(). If it is, then it probably wouldn't hurt to add a quick comment that says something like "We can ignore the type because it's included in the literal for this node type". If not some new test cases to validate that doesn't change.

@GuySartorelli
Copy link
Contributor Author

Closing since all of the identified items have been resolved - outstanding issues such as nested lists already have GitHub issues to track.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants