Conversation
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
I've been through the code which seems good to me so I can say I am taking responsibilities for it like my own. Still, another set of eyes would be good :) |
|
@robinst any thought ? |
robinst
left a comment
robinst
left a comment
There was a problem hiding this comment.
Some first comments, mostly around parsing and edge cases.
I was curious how GitHub implements this in cmark-gfm, and the answer seems to be that they don't:
github/cmark-gfm#350 (comment)
Alerts are not implemented in this project. They don't relate to their parser. They don't relate to syntax. So they're not here.
They are implemented as a post processing step on a sort of "DOM" version of the resulting document.
Can you explore what the implementation would look like as a PostProcessor instead? That has the advantage of not having to copy all the parsing logic from block quotes. (From the edge cases with nesting I commented about, it seems like they only look at top-level nodes.)
|
|
||
| public class AlertBlockParser extends AbstractBlockParser { | ||
|
|
||
| private static final Pattern ALERT_PATTERN = Pattern.compile("^\\[!([A-Z]+)]$"); |
There was a problem hiding this comment.
Note that in GitHub, they can be lowercase too:
> [!note]
> example
| * Custom types must: | ||
| * <ul> | ||
| * <li>Be UPPERCASE (e.g., "INFO", "SUCCESS")</li> | ||
| * <li>Not conflict with standard GFM types (NOTE, TIP, IMPORTANT, WARNING, CAUTION)</li> |
There was a problem hiding this comment.
Why not? E.g. if someone wanted to localize the title to another language on output, being able to override the title would make sense, no?
|
|
||
| @Test | ||
| public void emptyAlertWithNoContent() { | ||
| assertRendering("> [!NOTE]", |
There was a problem hiding this comment.
This doesn't match what GitHub does. It renders it as a normal block quote:
[!NOTE]
|
|
||
| @Test | ||
| public void alertWithOnlyWhitespaceContent() { | ||
| assertRendering("> [!TIP]\n> \n> ", |
There was a problem hiding this comment.
This doesn't match either:
[!TIP]
|
|
||
| @Test | ||
| public void lowercaseAlertTypeShouldNotMatch() { | ||
| assertRendering("> [!note]\n> This should be a blockquote", |
There was a problem hiding this comment.
Already commented on this earlier, but this is an alert on GitHub:
> [!note]
> This should be a blockquote
|
|
||
| @Test | ||
| public void mixedCaseAlertTypeShouldNotMatch() { | ||
| assertRendering("> [!Note]\n> This should be a blockquote", |
There was a problem hiding this comment.
This too:
> [!Note]
> This should be a blockquote
|
|
||
| @Test | ||
| public void nestedAlertMarkersAreTreatedAsText() { | ||
| assertRendering("> [!NOTE]\n> This is a note\n> [!WARNING]\n> This is still part of the note", |
There was a problem hiding this comment.
The test calls this nested, but nested is really this:
> [!NOTE]
> This is a note
>> [!WARNING]
>> This is still part of the note
Which GitHub, interestingly, renders as an alert with a normal blockquote inside. Even if we remove the outer alert, it stays as normal nested block quotes. Can you add those as tests?
This brings up another interesting case, can alerts be child blocks? E.g. this:
- > [!NOTE]
> Test
That's rendered as a list item with a block quote, not a list item with an alert inside. I think the implementation in this PR doesn't handle that the same way.
| String markdown = "> [!WARNING]\n> Be careful"; | ||
| Node document = PARSER.parse(markdown); | ||
| String rendered = MARKDOWN_RENDERER.render(document); | ||
| assertThat(rendered).contains("[!WARNING]"); |
There was a problem hiding this comment.
These tests should do roundtrip rendering like other markdown renderer tests do.
|
Thanks for the review! That all makes sense, will have a look soonish :) |
2 participants
Note
I have used Claude to code most of it, I am currently reviewing the generated code which seems to be overall valid. I've requested to follow the pattern of existing extensions (and compare to other implementations of this feature).
Summary
> [!NOTE],> [!WARNING], etc.)Closes #327