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

Fix alert blocks not following GitHub's behaviour with backslashes #31231

Closed
wants to merge 1 commit into from

Conversation

yardenshoham
Copy link
Member

@yardenshoham yardenshoham commented Jun 3, 2024

Previously, these blocks would only get rendered if the markdown was [!TYPE]. Now they also render with \[!TYPE\].

Before

image

After

image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 3, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 3, 2024
Previously, these blocks would only get rendered if the markdown was `[!TYPE]`. Now they also render with `\[!TYPE\]`.

Signed-off-by: Yarden Shoham <git@yardenshoham.com>
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Jun 3, 2024
@delvh
Copy link
Member

delvh commented Jun 3, 2024

Hmm… I'm torn on this one.
Normally I'd auto-approve anything that keeps us compatible with GitHubs behavior.
However, in this case, we have found such an edge case that I think GitHubs behavior is wrong:
When using markdown, you don't expect escaped quotes to behave like unescaped quotes.
So, to me, GitHubs behavior is rather a bug.

Should we cave to a bug in the dominant player?
I don't think so.
To me, it seems more likely that they'll decide one day that this behavior is a bug and fix it.
Additionally, I skimmed over the changes, they do appear to make the code less stable with some conditional logic that may affect future rendering in unexpected ways.

attentionParagraph := newAttentionParagraph(v, attentionType, g)
v.InsertBefore(v, firstChild, attentionParagraph)
if firstChild.ChildCount() == 0 {
v.RemoveChild(v, firstChild)
Copy link
Contributor

Choose a reason for hiding this comment

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

popAttentionTypeFromParagraph / popAttentionTypeFromMathBlock should handle the removal correctly, instead of removing the children here.

if firstParagraph.ChildCount() < 3 {
return ast.WalkContinue, nil
// make sure it's a !TYPE
if innerText[0] != '!' {
Copy link
Contributor

Choose a reason for hiding this comment

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

strings.HasPrefix to avoid panic if string is empty

if paragraph, ok := firstChild.(*ast.Paragraph); ok {
attentionType = popAttentionTypeFromParagraph(g, paragraph, reader)
} else {
mathBlock, ok := firstChild.(*math.Block)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe in the future we should use an "Extension" like "math" , instead of using "transform", then no need to play the type-casting trick with "math.Block" (just an idea, not a change suggestion 😁 )

@yardenshoham
Copy link
Member Author

@delvh convinced me

@wxiaoguang
Copy link
Contributor

But according to the reporter:

I noted this issue when the mdformat tool corrected the syntax by escaping the square brackets.

So I would suppose the \[!NOTE\] is also correct. I will propose a new fix.

@wxiaoguang
Copy link
Contributor

-> Make blockquote attention recognize more syntaxes #31240

@yardenshoham yardenshoham deleted the issues/31214 branch June 4, 2024 15:36
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Sep 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/go Pull requests that update Go code size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Markdown callouts/attention blocks do not follow GitHub's behaviour with backslashes
4 participants