Implement `AlertBlockSyntax` using `BlockquoteSyntax` #585

jonasfj · 2024-02-22T16:42:57Z

A proposed fix for #584

As I understand the alert-block syntax, it is an extension on the original blockquote support in markdown. Basically, it's a blockquote that is rendered differently.

That means that if alert-block syntax isn't supported a markdown parser will render it as a blockquote. This provides a nice graceful fallback. It also means that if you're implementing it, you really should implement it the same way you implement blockquote parsing.

This is an attempt at implementing AlertBlockSyntax using the same parsing logic as BlockquoteSyntax.

This is not pretty. Ideally, whether to support alert-syntax should be a boolean option passed to BlockquoteSyntax. Because an alert-syntax is a blockquote with extra bling. But that's not how this package is structured 🤣

In fact, it can probably be argued that trying to make a markdown parser that consists of composeable objects is extremely sketchy. Especially, when we let consumers of the package implement their own syntaxes and inject them into the whole mix. It's very hard to determine how this whole thing works (and what combinations of syntaxes will work). It would probably have been safer to pass in all the option as boolean configuration options that enable/disable a particular syntaxes in a single large parser.

As I understand the alert-block syntax, it is an extension on the orignal blockquote support in markdown. Basically, it's a blockquote that is rendered differently. That means that if alert-block syntax isn't supported a markdown parser will render it as a blockquote. This provides a nice graceful fallback. It also means that if you're implementing it, you really should implement it the same way you implement blockquote parsing.

jonasfj · 2024-02-22T16:43:36Z

Currently, running crash-test on this, though that won't reveal if we have bugs. Just that it doesn't crash 🤣

coveralls · 2024-02-22T16:45:20Z

Pull Request Test Coverage Report for Build 8007970317

Details

-1 of 18 (94.44%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.2%) to 96.345%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/src/block_syntaxes/blockquote_syntax.dart	17	18	94.44%

Totals
Change from base Build 7977582785:	0.2%
Covered Lines:	1529
Relevant Lines:	1587

💛 - Coveralls

jonasfj · 2024-02-22T21:02:29Z

No crashes scanning all markdown files in the latest version for all packages on pub.dev

Scanned 49735 / 49913 (skipped 0), found 0 issues
## Finished scanning
Scanned 49913 package versions in 0:23:20.241887
23:21 +1: All tests passed!

jonasfj · 2024-02-23T09:11:59Z

lib/src/block_syntaxes/alert_block_syntax.dart

-    return pattern.hasMatch(parser.current.content) &&
-        parser.lines.any((line) => _contentLineRegExp.hasMatch(line.content));


I can't make sense of this.

I suspect that parser.lines is the full text document.

So we're asking if both of the following are true:

(i) current line matches:
^\s{0,3}>\s{0,3}\\?\[!(note|tip|important|caution|warning)\\?\]\s*$
, and,

(ii) some line in the current document matches: >?\s?(.*)*.

If (i) is true, then I think (ii) is always trivially true.

I suspect this is here because there has to be a line on the form:
> [!note] and it has to be followed by a line > ... or a paragraph continuation.

But the implementation makes no sense, and it doesn't account for the paragraph continuation if it did.

I suspect that parser.lines is the full text document.

Hmm, that might explain #579 - parsing times being quadratic.

jonasfj · 2024-02-23T09:12:17Z

lib/src/block_syntaxes/alert_block_syntax.dart

-        continue;
-      }
-
-      final lastLine = childLines.last;


childLines can be empty at this point.

jonasfj · 2024-02-23T09:13:13Z

lib/src/block_syntaxes/alert_block_syntax.dart

-      final match = strippedContent.isEmpty
-          ? null
-          : _contentLineRegExp.firstMatch(strippedContent);


This makes it unable to distinguish between an empty line and a line containing only >, but these are very different at result in different behavior.

jonasfj · 2024-02-23T09:22:07Z

lib/src/block_syntaxes/blockquote_syntax.dart

+    );
+
+    if (isBlockquote) {
+      return Element('blockquote', children);


This is ugly, but I don't see a better way, unless we make BlockquoteSyntax do everything.

I suppose we could in theory make BlockquoteSyntax check if AlertBlockSyntax is enabled by doing:`

final isAlertSyntaxEnabled = parser.document.blockSyntaxes.any( (s) => s is AlertBlockSyntax, );

If we could give AlertBlockSyntax a factory singleton constructor we could do:

final isAlertSyntaxEnabled = parser.document.blockSyntaxes.contains( const AlertBlockSyntax(), );

If inside BlockquoteSyntax we had isAlertSyntaxEnabled then it could implement both. Ofcourse this would mean that enabling AlertBlockSyntax wouldn't work without enabling BlockquoteSyntax.
I'm not sure why we should ever allow such configuration, we surely don't intent to test it, but that's how the package is structured.

The point with implementing alert-syntax inside BlockquoteSyntax is that alert-syntax is really a post-processing step for blockquote. But I don't really see how package:markdown has the facilities to post-processing.

sigurdm · 2024-02-23T09:36:56Z

lib/src/block_syntaxes/blockquote_syntax.dart

+    // Until we've parse all the child lines, we can't actually know if this is
+    // a blockquote containing `[!note]` or if this is an alert-block.
+    //
+    // This is because `> [!note]` is not a valid alert-block!


Can you explain why this is not a valid alert block? because it is empty?

I don't know, that's how it behaves on github :D

But is it the emptiness that makes it invalid, or some other property?

Example of the empty case

----------- > [!note] -----------

[!note]

Example of the non-empty case

----------- > [!note] > Hello world -----------

Note
Hello world

Conclusion

If we have > [!note] it's not a special block. But if it's:

Followed by text that wants to belong to the previous paragraph, then it is. OR

Followed by lines like > ... indicating that whatever is there belongs inside the blockquote, then it also is special.

Otherwise, it's not special, just a normal blockquote.

Notice that > foo\nbar is the same as > foo\n> bar, the line that follows is considered to be a paragraph continuation of the previous line, so the blockquote continues.

So yes, as far as I can see, it's emptiness that decides it. Even if you have > [!note]\n> it'll still not be rendered as a special alert-block.

srawlins · 2024-03-07T04:31:08Z

I've just landed #593 as a fix for the crash, but I think this PR is probably going in a direction that would be good in the long term, so I'd like to keep it open. I'll try to review more tomorrow.

I looked at it today, and the only think I don't like is the same thing you don't like @jonasfj haha, which you elaborate on above.

I think there should be a way to "nicely" sit this syntax into the GFM extension, "on top" of the blockquote syntax.

In fact, it can probably be argued that trying to make a markdown parser that consists of composeable objects is extremely sketchy.

Haha this is really true. It's a weird concept, to have a plugable parser like this, based on an "ordering" of syntaxes, where we ask each in an order, "Can you parse this line? No? And you?" It's inherently flawed.

jonasfj · 2024-03-07T16:25:04Z

Haha this is really true. It's a weird concept, to have a plugable parser like this, based on an "ordering" of syntaxes, where we ask each in an order, "Can you parse this line? No? And you?" It's inherently flawed.

It's completely orthogonal, maybe it deserves a controversial proposal, where we can shoot down the idea :D

Filed #594

jonasfj requested a review from srawlins February 22, 2024 21:33

jonasfj commented Feb 23, 2024

View reviewed changes

sigurdm reviewed Feb 23, 2024

View reviewed changes

devoncarew mentioned this pull request Mar 6, 2024

fix a crash when parsing alert block syntax #593

Merged

1 task

jonasfj mentioned this pull request Mar 7, 2024

[controversial] Breaking API proposal #594

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `AlertBlockSyntax` using `BlockquoteSyntax` #585

Implement `AlertBlockSyntax` using `BlockquoteSyntax` #585

jonasfj commented Feb 22, 2024

jonasfj commented Feb 22, 2024

coveralls commented Feb 22, 2024

jonasfj commented Feb 22, 2024

jonasfj Feb 23, 2024

devoncarew Mar 6, 2024

jonasfj Feb 23, 2024

jonasfj Feb 23, 2024

jonasfj Feb 23, 2024

sigurdm Feb 23, 2024

jonasfj Feb 27, 2024

sigurdm Feb 27, 2024

jonasfj Mar 6, 2024

srawlins commented Mar 7, 2024

jonasfj commented Mar 7, 2024

		return pattern.hasMatch(parser.current.content) &&
		parser.lines.any((line) => _contentLineRegExp.hasMatch(line.content));

Implement AlertBlockSyntax using BlockquoteSyntax #585

Are you sure you want to change the base?

Implement AlertBlockSyntax using BlockquoteSyntax #585

Conversation

jonasfj commented Feb 22, 2024

jonasfj commented Feb 22, 2024

coveralls commented Feb 22, 2024

Pull Request Test Coverage Report for Build 8007970317

Details

💛 - Coveralls

jonasfj commented Feb 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Example of the empty case

Example of the non-empty case

Conclusion

srawlins commented Mar 7, 2024

jonasfj commented Mar 7, 2024

Implement `AlertBlockSyntax` using `BlockquoteSyntax` #585

Implement `AlertBlockSyntax` using `BlockquoteSyntax` #585