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

To add initial support for github md alerts #496

Merged
merged 1 commit into from
Sep 26, 2024
Merged

Conversation

prokod
Copy link
Contributor

@prokod prokod commented Aug 27, 2024

Closes #494

main.go Outdated Show resolved Hide resolved
pkg/mark/renderer/blockquote.go Outdated Show resolved Hide resolved
pkg/mark/renderer/blockquote.go Show resolved Hide resolved
@mrueg
Copy link
Collaborator

mrueg commented Aug 30, 2024

We're currently relicensing to Apache 2.0 only (before it was CommonsClause). Could you mention that you're okay with this being licensed under Apache 2.0?

@prokod
Copy link
Contributor Author

prokod commented Sep 1, 2024

@mrueg I am already a contributor and I have confirmed that I am ok with the lic change

Copy link
Collaborator

@mrueg mrueg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for the contribution!
I added a few minor nits and one ask: Can you also include it in the README.md under https://github.com/kovetskiy/mark?tab=readme-ov-file#block-quotes that it now supports Github Markdown Alerts?

pkg/mark/renderer/blockquote.go Outdated Show resolved Hide resolved
notePattern := regexp.MustCompile(`note|Note|NOTE`)
warnPattern := regexp.MustCompile(`warn|Warn|WARN`)
infoPattern := regexp.MustCompile(`(?i)info|important|caution`)
notePattern := regexp.MustCompile(`(?i)note`)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean moving caution to be together with warning ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just looking at the symbols and colour schemes, I think this mapping is the best to support Github Alerts:

Github Confluence
Note (blue I in circle) Info (blue I in circle)
Tip (green lightbulb) Tip (green checkmark in circle)
Important (purple exclamation mark in speech bubble Info (blue I in circle)
Warning (yellow exclamation mark in triangle) Note (yellow exclamation mark in triangle)
Caution (red exclamation mark in hexagon) Warning (red exclamation mark in hexagon)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review again after the last commit if it is matching your expectations :)

pkg/mark/testdata/quotes.md Show resolved Hide resolved
Copy link
Collaborator

@mrueg mrueg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is looking good.
I made a few suggestions. Can you rebase the changes on latest master branch and squash it afterwards?

README.md Outdated
@@ -262,7 +262,7 @@ some long code block
Block Quotes are converted to Confluence Info/Warn/Note box when the following conditions are met

1. The BlockQuote is on the root level of the document (not nested)
1. The first line of the BlockQuote contains one of the following patterns `Info/Warn/Note`
1. 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) `[!INFO]/[!NOTE]/[!WARN][!IMPORTANT]`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. 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) `[!INFO]/[!NOTE]/[!WARN][!IMPORTANT]`
1. 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]`

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also include this suggestion?

patternMap: map[string]*regexp.Regexp{
"info": regexp.MustCompile(`(?i)^\!info|important`),
"note": regexp.MustCompile(`(?i)^\!note`),
"warn": regexp.MustCompile(`(?i)^\!warning|caution`),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"warn": regexp.MustCompile(`(?i)^\!warning|caution`),
"warn": regexp.MustCompile(`(?i)^\!(warning|caution)`),

pkg/mark/renderer/blockquote.go Outdated Show resolved Hide resolved
@mrueg
Copy link
Collaborator

mrueg commented Sep 25, 2024

Thanks for the contribution!
Looks good overall, one last thing: Can you include #496 (comment) and then squash the commits so I can merge it?

@prokod
Copy link
Contributor Author

prokod commented Sep 25, 2024

@mrueg please check before squashing

@mrueg
Copy link
Collaborator

mrueg commented Sep 25, 2024

Thanks, those changes look good!

@mrueg mrueg merged commit 035db7b into kovetskiy:master Sep 26, 2024
5 checks passed
@mrueg
Copy link
Collaborator

mrueg commented Sep 26, 2024

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Github Alerts Markdown
2 participants