Conversation
|
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
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.)
...rk-ext-gfm-alerts/src/main/java/org/commonmark/ext/gfm/alerts/internal/AlertBlockParser.java
Outdated
Show resolved
Hide resolved
commonmark-ext-gfm-alerts/src/main/java/org/commonmark/ext/gfm/alerts/AlertsExtension.java
Outdated
Show resolved
Hide resolved
commonmark-ext-gfm-alerts/src/test/java/org/commonmark/ext/gfm/alerts/AlertsEdgeCasesTest.java
Outdated
Show resolved
Hide resolved
commonmark-ext-gfm-alerts/src/test/java/org/commonmark/ext/gfm/alerts/AlertsEdgeCasesTest.java
Outdated
Show resolved
Hide resolved
commonmark-ext-gfm-alerts/src/test/java/org/commonmark/ext/gfm/alerts/AlertsEdgeCasesTest.java
Outdated
Show resolved
Hide resolved
commonmark-ext-gfm-alerts/src/test/java/org/commonmark/ext/gfm/alerts/AlertsEdgeCasesTest.java
Outdated
Show resolved
Hide resolved
commonmark-ext-gfm-alerts/src/test/java/org/commonmark/ext/gfm/alerts/AlertsEdgeCasesTest.java
Outdated
Show resolved
Hide resolved
...k-ext-gfm-alerts/src/test/java/org/commonmark/ext/gfm/alerts/AlertsMarkdownRendererTest.java
Outdated
Show resolved
Hide resolved
|
Thanks for the review! That all makes sense, will have a look soonish :) |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
I made all the changes, I am currently generating a small jbang script to generate the spec from gh api result, not needed but cool :) |
Add a jbang script (generate-alerts-spec.java) that generates
alerts-spec.txt from a template (alerts-spec-template.md) by rendering
each example through the GitHub Markdown API and normalizing the HTML.
Configure AlertsSpecTest with softbreak("<br>") to match GitHub's
rendering, reducing the normalizations needed in the generator.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@robinst it is ready for another review thanks! |
robinst
left a comment
There was a problem hiding this comment.
More feedback. Getting closer, but I think we can still simplify.
Neat idea to generate the test data by using the gh CLI.
...t-gfm-alerts/src/main/java/org/commonmark/ext/gfm/alerts/internal/AlertHtmlNodeRenderer.java
Outdated
Show resolved
Hide resolved
...k-ext-gfm-alerts/src/main/java/org/commonmark/ext/gfm/alerts/internal/AlertNodeRenderer.java
Outdated
Show resolved
Hide resolved
...-ext-gfm-alerts/src/main/java/org/commonmark/ext/gfm/alerts/internal/AlertPostProcessor.java
Outdated
Show resolved
Hide resolved
...-ext-gfm-alerts/src/main/java/org/commonmark/ext/gfm/alerts/internal/AlertPostProcessor.java
Outdated
Show resolved
Hide resolved
...-ext-gfm-alerts/src/main/java/org/commonmark/ext/gfm/alerts/internal/AlertPostProcessor.java
Outdated
Show resolved
Hide resolved
...-ext-gfm-alerts/src/main/java/org/commonmark/ext/gfm/alerts/internal/AlertPostProcessor.java
Outdated
Show resolved
Hide resolved
commonmark-ext-gfm-alerts/src/main/java/org/commonmark/ext/gfm/alerts/AlertsExtension.java
Outdated
Show resolved
Hide resolved
commonmark-ext-gfm-alerts/src/main/java/org/commonmark/ext/gfm/alerts/AlertsExtension.java
Outdated
Show resolved
Hide resolved
commonmark-ext-gfm-alerts/src/main/java/org/commonmark/ext/gfm/alerts/AlertsExtension.java
Outdated
Show resolved
Hide resolved
- Replace AbstractVisitor with direct Document child iteration (only top-level block quotes are alerts, no need to walk the full tree) - Remove unnecessary isContentWhitespaceOnly method (the parser never produces content nodes for whitespace-only block quote continuations) - Remove redundant markerText/literal variables, use textNode.getLiteral() - Restructure afterMarker checks to avoid 3x instanceof repetition - Add Locale.ROOT to toUpperCase() calls (Turkish locale safety) - Use var throughout all main source files - Use single get()+null check instead of containsKey()+get() in renderer - Add proper Node import in AlertNodeRenderer, remove FQN references - LinkedHashMap → HashMap in AlertsExtension (ordering not needed) - Update README: fix outdated custom type docs, align examples with AlertsExample.java - Add screenshots showing rendered alerts with and without icons Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
All review feedback has been addressed: PostProcessor simplification:
Code cleanup:
README:
|
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
PostProcessor(transformsBlockQuotenodes intoAlertnodes after parsing)[!note],[!Note]work like[!NOTE])Test approach
AlertsSpecTest— parameterized spec-based tests fromalerts-spec.txt, expectations verified against GitHub Markdown API (gh api markdown -f mode=gfm)AlertsTest— custom types, builder validation, AST assertionsAlertsMarkdownRendererTest— roundtrip rendering testsChanges from review
BlockParsertoPostProcessorapproachTextContentRenderer(core handles it)Alert(immutable type, no public setter)STANDARD_TYPESfromAlertnode toAlertsExtensiongetCustomTypes()config leak from public APIcapitalizefallback)Closes #327