Conversation
61d83fd to
0d39702
Compare
0d39702 to
c84b17c
Compare
b660f8b to
9abfbdd
Compare
|
Hi @prokod ! Thank you for your contribution! I reviewed the comments and I think all of them have been addressed, the only problem is the branch conflict. The only conflicting file is This comment is just a reminder for you, this is a great feature and there seems to be only a minimal blocking right now. I hope you find available time to fix the issue 😊 |
There was a problem hiding this comment.
Pull request overview
This pull request implements a transformer-based approach for handling GitHub Alerts ([!NOTE], [!TIP], [!WARNING], [!CAUTION], [!IMPORTANT]) in markdown, converting them to user-friendly titles in Confluence macros. This supersedes PR #538 and addresses the issue of unfriendly GitHub Alert notation by transforming the syntax at the AST level before rendering.
Changes:
- Implements a new GHAlertsTransformer that processes GitHub Alert syntax during AST transformation, replacing
[!TYPE]markers with capitalized titles (e.g., "Note", "Tip") - Adds dedicated renderers (ConfluenceGHAlertsBlockQuoteRenderer and enhanced ConfluenceTextRenderer) that work with the transformer to produce proper Confluence macros
- Preserves backward compatibility through separate legacy implementations and maintains support for traditional
info:,note:,tip:,warning:blockquote syntax
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| transformer/gh_alerts.go | New transformer that identifies GitHub Alert patterns in blockquotes and sets AST attributes for rendering |
| transformer/README.md | Documentation explaining the GitHub Alerts transformer functionality and usage |
| renderer/text.go | Enhanced text renderer that checks for replacement-content attributes from the transformer |
| renderer/text_legacy.go | Preserved legacy text renderer without GitHub Alerts support for backward compatibility |
| renderer/gh_alerts_blockquote.go | New blockquote renderer that handles both GitHub Alerts and legacy blockquote syntax |
| renderer/blockquote.go | Removed GitHub Alerts classification code that's now handled by the transformer |
| markdown/markdown.go | Integration layer with new ConfluenceExtension using the transformer, legacy extension preservation, and helper functions |
| markdown/transformer_comparison_test.go | Comprehensive tests comparing transformer and legacy approaches, plus compatibility tests |
| testdata/quotes.html | Updated expected output showing transformed GitHub Alerts with user-friendly titles |
| testdata/quotes-stripnewlines.html | Updated expected output for stripnewlines mode |
| testdata/quotes-droph1.html | Updated expected output for droph1 mode |
| README.md | Updated documentation with GitHub Alerts examples and usage information |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // We'll return attachments separately - caller handles this | ||
| return string(html), []attachment.Attachment{} |
There was a problem hiding this comment.
The helper function compileMarkdownWithExtension always returns an empty attachment slice, but the callers ignore this return value and use their own attachment handling. This works correctly, but the return value in the helper is misleading. Consider either removing the attachment return value from this helper function or properly collecting attachments from the extension parameter.
| @@ -87,6 +92,7 @@ func (r *ConfluenceTextRenderer) renderText(w util.BufWriter, source []byte, nod | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| return ast.WalkContinue, nil | |||
| } | |||
There was a problem hiding this comment.
There's significant code duplication between ConfluenceTextRenderer.renderText and ConfluenceTextLegacyRenderer.renderText. The only difference is the GitHub Alerts replacement content check in lines 43-52. Consider refactoring to share the common East Asian line break handling logic, or add a comment explaining why the duplication is necessary.
| // Look for the pattern: [!ALERTTYPE] | ||
| // We need to check for three consecutive text nodes: "[", "!ALERTTYPE", "]" | ||
| currentNode := firstText | ||
| var nodes []ast.Node | ||
|
|
||
| // Collect up to 3 text nodes | ||
| for i := 0; i < 3 && currentNode != nil && currentNode.Kind() == ast.KindText; i++ { | ||
| nodes = append(nodes, currentNode) | ||
| currentNode = currentNode.NextSibling() | ||
| } | ||
|
|
||
| if len(nodes) < 3 { | ||
| return "" | ||
| } | ||
|
|
||
| leftText := nodes[0].(*ast.Text) | ||
| middleText := nodes[1].(*ast.Text) | ||
| rightText := nodes[2].(*ast.Text) | ||
|
|
||
| leftContent := string(leftText.Segment.Value(reader.Source())) | ||
| middleContent := string(middleText.Segment.Value(reader.Source())) | ||
| rightContent := string(rightText.Segment.Value(reader.Source())) | ||
|
|
||
| // Check for the exact pattern | ||
| if leftContent == "[" && rightContent == "]" && strings.HasPrefix(middleContent, "!") { | ||
| alertType := strings.ToLower(strings.TrimPrefix(middleContent, "!")) | ||
|
|
||
| // Validate it's a recognized GitHub Alert type | ||
| switch alertType { | ||
| case "note", "tip", "important", "warning", "caution": | ||
| return alertType | ||
| } | ||
| } | ||
|
|
||
| return "" |
There was a problem hiding this comment.
The extractAlertType and removeAlertSyntax methods both collect three text nodes and check if there are at least 3. However, there's a subtle issue: if exactly 3 text nodes are found but they don't match the pattern (e.g., leftContent != "["), the function correctly returns early without modifying anything. However, if there are more than 3 text nodes where the first 3 don't match but later nodes might, those won't be checked. This is likely the intended behavior for GitHub Alerts (which should be at the very start), but consider adding a comment explaining this design choice.
| <ac:structured-macro ac:name="info"> | ||
| <ac:parameter ac:name="icon">true</ac:parameter> | ||
| <ac:rich-text-body> | ||
| <p>Note This is a note alert with <strong>markdown</strong> formatting.</p> |
There was a problem hiding this comment.
The example output in the documentation shows "Note This is a note alert" on a single line, but based on the actual implementation and test data, the title "Note" should be in its own paragraph tag. The example should show:
<p>Note</p>
<p>This is a note alert with <strong>markdown</strong> formatting.</p>instead of having them on the same line.
| <p>Note This is a note alert with <strong>markdown</strong> formatting.</p> | |
| <p>Note</p> | |
| <p>This is a note alert with <strong>markdown</strong> formatting.</p> |
| if leftContent == "[" && rightContent == "]" && strings.HasPrefix(middleContent, "!") { | ||
| alertType := strings.ToLower(strings.TrimPrefix(middleContent, "!")) | ||
|
|
||
| // Validate it's a recognized GitHub Alert type | ||
| switch alertType { | ||
| case "note", "tip", "important", "warning", "caution": | ||
| return alertType | ||
| } | ||
| } |
There was a problem hiding this comment.
The alert type extraction uses strings.ToLower which handles case-insensitive matching correctly. However, the implementation doesn't handle whitespace. For example, [! NOTE] (with a space after the exclamation mark) or [!NOTE ] (with trailing space) would not be recognized. While this matches GitHub's strict syntax, consider documenting this behavior or adding a test case to explicitly verify that whitespace variations are rejected.
| 2. The first line of the BlockQuote contains one of the following patterns `Info/Warn/Note` or [Github MD Alerts style](https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax#alerts) `[!NOTE]/[!TIP]/[!IMPORTANT]/[!WARNING]/[!CAUTION]` | ||
|
|
||
| | Github Alerts | Confluence | Description | |
There was a problem hiding this comment.
Minor spelling inconsistency: "Github" should be "GitHub" (with capital H) for consistency with the company's branding. This appears in the heading "Github MD Alerts style" and the table headers.
| 2. The first line of the BlockQuote contains one of the following patterns `Info/Warn/Note` or [Github MD Alerts style](https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax#alerts) `[!NOTE]/[!TIP]/[!IMPORTANT]/[!WARNING]/[!CAUTION]` | |
| | Github Alerts | Confluence | Description | | |
| 2. The first line of the BlockQuote contains one of the following patterns `Info/Warn/Note` or [GitHub MD Alerts style](https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax#alerts) `[!NOTE]/[!TIP]/[!IMPORTANT]/[!WARNING]/[!CAUTION]` | |
| | GitHub Alerts | Confluence | Description | |
| // CompileMarkdown compiles markdown to Confluence Storage Format with GitHub Alerts support | ||
| // This is the main function that now uses the enhanced GitHub Alerts transformer by default | ||
| // for superior processing of [!NOTE], [!TIP], [!WARNING], [!CAUTION], [!IMPORTANT] syntax | ||
| func CompileMarkdown(markdown []byte, stdlib *stdlib.Lib, path string, cfg types.MarkConfig) (string, []attachment.Attachment) { | ||
| // Use the enhanced GitHub Alerts extension for better processing | ||
| ghAlertsExtension := NewConfluenceExtension(stdlib, path, cfg) | ||
| html, _ := compileMarkdownWithExtension(markdown, ghAlertsExtension, "rendering markdown with GitHub Alerts support:\n%s") | ||
| return html, ghAlertsExtension.Attachments | ||
| } |
There was a problem hiding this comment.
Changing the default behavior of CompileMarkdown to use GitHub Alerts transformer is a breaking change. Previously, blockquotes containing [!NOTE] would render with that text literally visible. Now they will be converted to macros with "Note" as the title. While this is likely the desired behavior for most users (and matches GitHub's rendering), consider:
- Documenting this breaking change in release notes
- Providing a migration guide for users who need the old behavior
- Consider if a configuration flag should control this behavior by default
| // LegacyMarkdownCompiler implements the original compilation without GitHub Alerts transformer | ||
| type LegacyMarkdownCompiler struct{} | ||
|
|
||
| func (c *LegacyMarkdownCompiler) Compile(markdown []byte, stdlib *stdlib.Lib, path string, cfg types.MarkConfig) (string, []attachment.Attachment) { | ||
| return CompileMarkdownLegacy(markdown, stdlib, path, cfg) | ||
| } | ||
|
|
||
| // GHAlertsMarkdownCompiler implements compilation with GitHub Alerts transformer | ||
| type GHAlertsMarkdownCompiler struct{} | ||
|
|
||
| func (c *GHAlertsMarkdownCompiler) Compile(markdown []byte, stdlib *stdlib.Lib, path string, cfg types.MarkConfig) (string, []attachment.Attachment) { | ||
| return CompileMarkdownWithTransformer(markdown, stdlib, path, cfg) | ||
| } | ||
|
|
||
| // CompileMarkdownWithDecorator allows choosing between legacy and GitHub Alerts approaches | ||
| // This provides a flexible way to switch implementations based on configuration or feature flags | ||
| func CompileMarkdownWithDecorator(markdown []byte, stdlib *stdlib.Lib, path string, cfg types.MarkConfig, useGHAlerts bool) (string, []attachment.Attachment) { | ||
| var compiler MarkdownCompiler | ||
|
|
||
| if useGHAlerts { | ||
| compiler = &GHAlertsMarkdownCompiler{} | ||
| log.Tracef(nil, "using GitHub Alerts transformer compiler") | ||
| } else { | ||
| compiler = &LegacyMarkdownCompiler{} | ||
| log.Tracef(nil, "using legacy compiler") | ||
| } | ||
|
|
||
| return compiler.Compile(markdown, stdlib, path, cfg) | ||
| } |
There was a problem hiding this comment.
The decorator pattern implementation (MarkdownCompiler interface, LegacyMarkdownCompiler, GHAlertsMarkdownCompiler, and CompileMarkdownWithDecorator function) appears to be unused code. If this was intended for future extensibility or as an alternative API, consider documenting its purpose. Otherwise, consider removing this unused code to reduce maintenance burden.
| // LegacyMarkdownCompiler implements the original compilation without GitHub Alerts transformer | |
| type LegacyMarkdownCompiler struct{} | |
| func (c *LegacyMarkdownCompiler) Compile(markdown []byte, stdlib *stdlib.Lib, path string, cfg types.MarkConfig) (string, []attachment.Attachment) { | |
| return CompileMarkdownLegacy(markdown, stdlib, path, cfg) | |
| } | |
| // GHAlertsMarkdownCompiler implements compilation with GitHub Alerts transformer | |
| type GHAlertsMarkdownCompiler struct{} | |
| func (c *GHAlertsMarkdownCompiler) Compile(markdown []byte, stdlib *stdlib.Lib, path string, cfg types.MarkConfig) (string, []attachment.Attachment) { | |
| return CompileMarkdownWithTransformer(markdown, stdlib, path, cfg) | |
| } | |
| // CompileMarkdownWithDecorator allows choosing between legacy and GitHub Alerts approaches | |
| // This provides a flexible way to switch implementations based on configuration or feature flags | |
| func CompileMarkdownWithDecorator(markdown []byte, stdlib *stdlib.Lib, path string, cfg types.MarkConfig, useGHAlerts bool) (string, []attachment.Attachment) { | |
| var compiler MarkdownCompiler | |
| if useGHAlerts { | |
| compiler = &GHAlertsMarkdownCompiler{} | |
| log.Tracef(nil, "using GitHub Alerts transformer compiler") | |
| } else { | |
| compiler = &LegacyMarkdownCompiler{} | |
| log.Tracef(nil, "using legacy compiler") | |
| } | |
| return compiler.Compile(markdown, stdlib, path, cfg) | |
| } | |
| // Note: legacy decorator-based Markdown compiler types and helpers that wrapped | |
| // CompileMarkdownLegacy and CompileMarkdownWithTransformer were removed because | |
| // they were unused thin wrappers. If a decorator-style API is needed again in | |
| // the future, it can be reintroduced from version control history. |
| ### Block Quotes | ||
|
|
||
| Block Quotes are converted to Confluence Info/Warn/Note box when the following conditions are met | ||
| Mark now supports GitHub Alerts to create highlighted information boxes using Github Flavored Markdown syntax! |
There was a problem hiding this comment.
Minor spelling inconsistency: "Github" should be "GitHub" (with capital H) for consistency with the company's branding.
| Mark now supports GitHub Alerts to create highlighted information boxes using Github Flavored Markdown syntax! | |
| Mark now supports GitHub Alerts to create highlighted information boxes using GitHub Flavored Markdown syntax! |
b1475d3 to
b5b7142
Compare
To update transformer renderers - compatibility work with original approach integration - step 1 integration - step 2 integration - step 3 integration - step 4 PR review updates
b5b7142 to
8939a10
Compare
|
Hi @br-arruda, I fixed the README.md. There is one test failing |
Supersedes PR #538 and based on the discussion there