search

dart-lang / markdown

A Dart markdown library
https://pub.dev/packages/markdown
BSD 3-Clause "New" or "Revised" License
440 stars 200 forks source link

Implement `AlertBlockSyntax` using `BlockquoteSyntax` #585

Open jonasfj opened 4 months ago

jonasfj commented 4 months ago

A proposed fix for https://github.com/dart-lang/markdown/issues/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 :rofl:

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.

jonasfj commented 4 months ago

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

coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 8007970317

Details

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/src/block_syntaxes/blockquote_syntax.dart 17 18 94.44%
<!-- Total: 17 18 94.44% -->
Totals Coverage Status
Change from base Build 7977582785: 0.2%
Covered Lines: 1529
Relevant Lines: 1587
💛 - Coveralls
jonasfj commented 4 months ago

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!
srawlins commented 4 months ago

I've just landed https://github.com/dart-lang/markdown/pull/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 commented 4 months ago

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 https://github.com/dart-lang/markdown/issues/594