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

Blockquote tags #869

Merged
merged 9 commits into from
Apr 1, 2024
Merged

Blockquote tags #869

merged 9 commits into from
Apr 1, 2024

Conversation

Martin1887
Copy link
Collaborator

@Martin1887 Martin1887 commented Mar 24, 2024

Fixes #718.

The tags matching in scanners.rs and the detection of being currently in a blockquote of firstpass.rs could probably be improved.

More importantly, one of the tests currently fails with two consecutive blockquotes because the end of the previous one is not detected, so tags are not searched in the latest one.

pulldown-cmark/src/scanners.rs Outdated Show resolved Hide resolved
@Martin1887
Copy link
Collaborator Author

Martin1887 commented Mar 28, 2024

Thanks! I will apply your reviews this week. About the options flag, I think it is not necessary for these kind of things that do not produce breaking changes (who would want to literally put in the first line of a blockquote [!TIP]?).

@ollpu
Copy link
Collaborator

ollpu commented Mar 28, 2024

So GFM seems to allow having empty lines (containing only whitespace) and indentation in the blockquote before the tag:

>        
>  [!NOTE] 
>  Content
>

Note

Content

This seems like a style we also want to support (having extra empty lines or extra spaces).

It's also pretty obvious that GitHub does this in post processing. I thought maybe we should do it like that as well. This isn't great behavior though:

> \[!NOTE\]\
> No way to opt out?

Note

No way to opt out?

So maybe not.


Regarding the Option flag, I feel that we should keep everything strictly in line with Commonmark when no option flags are active. It might not be a breaking change for existing content in practice, but the fact that an untrusted user can now use this syntax can be unexpected for a maintainer. Maybe the classes have different existing meanings. It also causes problems with differential fuzzing and the like.

Some of these miscellaneous GFM features could be grouped under the same GFM flag though.

@ollpu
Copy link
Collaborator

ollpu commented Mar 28, 2024

As for the classes emitted by the renderer, maybe they should be more specific? GitHub uses markdown-alert markdown-alert-note etc.

@notriddle
Copy link
Collaborator

notriddle commented Mar 28, 2024

Looking at a few other implementations that support this syntax, most of them don't seem to let you do the blank-line-before-the-tag thing.

  • .NET DocFX, where I think GitHub copied this syntax from, actually lets callouts interrupt each other.
  • commonmark-hs also sees blank lines and backslashes as disabling this.
  • I tried it in Obsidian, which might be where this syntax came from? They don't have a public test suite, though, so it's harder to deliberately align with them. They don't let you start with a blank line, though.

@Martin1887
Copy link
Collaborator Author

OK, I forgot there is a flag for GFM 😅, it must be used. I will handle the any number of spaces and tests will be added for this case too. Regarding the other implementations don't support an empty line before, I think that should not be supported.

Thanks!

@Martin1887 Martin1887 closed this Mar 28, 2024
@ollpu
Copy link
Collaborator

ollpu commented Mar 28, 2024

@notriddle Gotcha. The stricter semantics make sense then. Yet another feature where GitHub's lazy implementations has very different semantics from the rest :D

@Martin1887 Sorry I was unclear, there's no existing flag for GFM, but one could be added if there are many miscellaneous GFM-esque features. Maybe it doesn't make sense at this point though.

@ollpu
Copy link
Collaborator

ollpu commented Mar 28, 2024

Also I guess the extra spaces aren't supported by most other implementations either. commonmark-hs only accepts one space after, but none before. So the behavior in this PR is probably mostly ok.

@Martin1887 Martin1887 reopened this Mar 28, 2024
@Martin1887
Copy link
Collaborator Author

I think that all issues are solved now and the pull request can be merged. I finally added an option tag for GFM misc features, currently only for blockquote tags. Thanks!

The 0.11 release is closer now!

@Martin1887 Martin1887 linked an issue Mar 30, 2024 that may be closed by this pull request
Copy link
Collaborator

@notriddle notriddle left a comment

Choose a reason for hiding this comment

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

I've put together a bunch of fixes for these bugs. It also could use more test cases.

notriddle@2b7aa68

Does most of the implementation look reasonable?

pulldown-cmark/src/scanners.rs Outdated Show resolved Hide resolved
pulldown-cmark/src/scanners.rs Outdated Show resolved Hide resolved
@Martin1887
Copy link
Collaborator Author

Thanks, I have checked your code and merge it. The only thing I have changed is a comment about the while loop is wrong, left after copying my reviewed code I guess.

So this should be good to be merged now.

@Martin1887 Martin1887 merged commit 2540d40 into branch_0.11 Apr 1, 2024
2 checks passed
@notriddle notriddle deleted the blockquote_tags branch April 1, 2024 15:35
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.

Note, important and warning blockquote
3 participants