Create DelimiterSyntax to replace TagSyntax #407

chenzhiguang · 2022-04-05T14:20:17Z

Introduces a new syntax DelimiterSyntax which has the same parsing strategy as TagSyntax, but more flexible.

For example:

const text = '#####tag#####';

final result = markdownToHtml(text, inlineSyntaxes: [
  DelimiterSyntax('#+', requiresDelimiterRun: true, tags: [
    DelimiterTag('flash', 5),
  ]),
]

output:

<p><flash>tag</flash></p>

This PR also add anther syntax EmphasisSyntax, which replaces TagSyntax(r'\*+', requiresDelimiterRun: true) and TagSyntax(r'_+', requiresDelimiterRun: true)

Fixes Incorrect Parsing of Strikethrough Tags When Mixed with Emphasis and Order Changes #357

And create a new TagSyntax extends from DelimiterSyntax

kevmoo · 2022-04-05T18:46:52Z

This is going to need a changelog entry – right?

chenzhiguang · 2022-04-05T21:17:31Z

This is going to need a changelog entry – right?

Yes

Because + Intraword emphasis with * is permitted + Intraword emphasis with _ is not permitted

Fix: #357

chenzhiguang · 2022-04-06T14:39:41Z

Sorry to keep committing new stuffs to this PR.
Now I am done. I will create another PR if I find any new issues or want to commit anything new.

srawlins

Note that this is a breaking change, even leaving TagSyntax intact: Some fields now return DelimiterSyntax instead of TagSyntax, and StrikethroughSyntax no longer extends TagSyntax.

If you like, you can add a CHANGELOG entry, and bump the major version number in pubspec.yaml and lib/src/version.dart.

srawlins · 2022-04-08T20:12:15Z

lib/src/inline_parser.dart

@@ -972,7 +1016,7 @@ class LinkSyntax extends TagSyntax {
  @override
  Node? close(
      InlineParser parser, covariant SimpleDelimiter opener, Delimiter? closer,
-      {required List<Node> Function() getChildren}) {
+      {String? tag, required List<Node> Function() getChildren}) {


Style nit: please add a trailing comma here, so that each parameter appears on its own line.

srawlins · 2022-04-08T20:12:53Z

lib/src/inline_parser.dart

-      {required List<Node> Function() getChildren}) {
-    return Element('del', getChildren());
-  }
+/// Parses `==mark==` to `<mark>mark</mark>`.


Can you add some documentation about where this is coming from? We need specifications which we can follow in order to support special syntaxes.

I did not find a common used specification about it.

My idea is all the syntaxes extend DelimiterSyntax with requiresDelimiterRun = true follow the commonmark specification of Emphasis and strong emphasis: https://spec.commonmark.org/0.30/#emphasis-and-strong-emphasis

I don't think we can support an unspecified, or loosely specified syntax. Even if it is not a complicated one, it adds maintenance burden, and can create breaking changes when a well-specified, popular syntax does emerge, and is slightly different from what we've implemented. As is noted in the issue, some implementations seem to support different delimiters for highlighting, so I think this is too under-specified to accept.

I'll comment more on the issue.

srawlins · 2022-04-08T20:15:00Z

lib/src/inline_parser.dart

-  /// Create a new [TagSyntax] which matches text on [pattern].
+  final List<DelimiterTag>? tags;
+
+  /// Create a new [DelimiterSyntax] which matches text on [pattern].


Nit: "Creates" instead of "Create"

https://dart.dev/guides/language/effective-dart/documentation#prefer-starting-function-or-method-comments-with-third-person-verbs

Thanks! Fixed

srawlins · 2022-04-08T20:16:09Z

lib/src/inline_parser.dart

+class DelimiterTag {
+  DelimiterTag(this.tag, this.indicators);
+
+  // Tag name of AST Element


Grammar nit: "Tag name of the HTML element." (ending with a period)

srawlins · 2022-04-08T20:17:13Z

lib/src/inline_parser.dart

+  final String tag;
+
+  /// The length of [indicators]
+  final int indicators;


The name of this field is probably not the best (as seen by the doc comment above it).

I would expect a field named 'indicators' to the String perhaps, or a List. This should be renamed to something like... 'indicatorLength'. If that is too long, I'd be open to other ideas...

I have changed it to indicatorLength. Yes, indicatorLength is too long, this was the reason why I named it indicators.

srawlins · 2022-04-08T20:21:03Z

lib/src/inline_parser.dart

-      {required List<Node> Function() getChildren}) {
-    var strong = opener.length >= 2 && closer.length >= 2;
-    return Element(strong ? 'strong' : 'em', getChildren());
+      {required String tag, required List<Node> Function() getChildren}) {


Style nit: please add a trailing comma here, so that each parameter appears on its own line.

Fixed. I thought we prefer to use this no trailing comma style.

I actually don't have a strong preference. Except when we have some long parameters (like with the required keyword), and now they're creeping onto a third line. In the analyzer codebase we've been moving towards using trailing commas more. It's not a hard rule yet.

chenzhiguang · 2022-04-08T22:18:37Z

Note that this is a breaking change, even leaving TagSyntax intact: Some fields now return DelimiterSyntax instead of TagSyntax, and StrikethroughSyntax no longer extends TagSyntax.

If you like, you can add a CHANGELOG entry, and bump the major version number in pubspec.yaml and lib/src/version.dart.

Done!

chenzhiguang · 2022-04-14T07:02:45Z

Will we merge this PR? I'm waiting for the merge to continue with other tasks to avoid a lot of conflicts. Thanks!

srawlins

I think that DelimiterSyntax is a fine improvement over TagSyntax, and I think we can definitely merge this PR with that feature.

But I don't think the HighlightSyntax should be a part of all this. The DelimiterSyntax definitely makes HighlightSyntax easier, so that others can implement this syntax, and perhaps other extensions for other Markdown flavors.

chenzhiguang · 2022-04-14T16:37:06Z

Thanks! HighlightSyntax has been removed.

Yes, I agree, we should only focus on the standard syntaxes / flavors.

srawlins

Very nice feature, thanks!

chenzhiguang added 7 commits April 4, 2022 17:47

some minor update

a5a5a49

Rename TagSyntax to DelimiterSyntax

0cf800a

And create a new TagSyntax extends from DelimiterSyntax

Introduce DelimiterTag to define tags

7fe563f

Create EmphasisSyntax

b413776

ignore the indicators shorter than required.

e8d2c14

Create HighlightSyntax

71b0993

Fix markdown_test formatting

d099248

kevmoo requested a review from srawlins April 5, 2022 18:46

chenzhiguang added 3 commits April 6, 2022 10:27

Remove EmphasisSyntax

4ac92f3

Because + Intraword emphasis with * is permitted + Intraword emphasis with _ is not permitted

Fix the logic handling allowIntraWord

cc5591c

Fix: #357

Create EmphasisSyntax

10e1743

Make EmphasisSyntax easier to use

0c7da2c

srawlins requested changes Apr 8, 2022

View reviewed changes

chenzhiguang added 3 commits April 8, 2022 23:04

Rename indicators to indicatorLength and some small fixes

8c11cca

Add specification reference to HighlightSyntax

afd6bf0

Add a CHANGELOG entry and bump the major version number

9d4ff64

srawlins requested changes Apr 14, 2022

View reviewed changes

srawlins mentioned this pull request Apr 14, 2022

Support for text highlight. #365

Open

Remove HighlightSyntax

29db547

Remove HighlightSyntax from CHANGELOG

abc1917

srawlins approved these changes Apr 14, 2022

View reviewed changes

srawlins merged commit 6d39147 into dart-lang:master Apr 14, 2022

chenzhiguang deleted the Improve-TagSyntax branch April 18, 2022 14:57

roubachof pushed a commit to roubachof/markdown that referenced this pull request Jul 1, 2022

Create DelimiterSyntax to replace TagSyntax (dart-lang#407)

c8bbe95

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create DelimiterSyntax to replace TagSyntax #407

Create DelimiterSyntax to replace TagSyntax #407

chenzhiguang commented Apr 5, 2022 •

edited

Loading

kevmoo commented Apr 5, 2022

chenzhiguang commented Apr 5, 2022

chenzhiguang commented Apr 6, 2022

srawlins left a comment

srawlins Apr 8, 2022

chenzhiguang Apr 8, 2022

srawlins Apr 8, 2022

chenzhiguang Apr 8, 2022

srawlins Apr 14, 2022

srawlins Apr 8, 2022

chenzhiguang Apr 8, 2022

srawlins Apr 8, 2022

chenzhiguang Apr 8, 2022

srawlins Apr 8, 2022

chenzhiguang Apr 8, 2022

srawlins Apr 8, 2022

chenzhiguang Apr 8, 2022

srawlins Apr 8, 2022

chenzhiguang commented Apr 8, 2022 •

edited

Loading

chenzhiguang commented Apr 14, 2022

srawlins left a comment

chenzhiguang commented Apr 14, 2022

srawlins left a comment

Create DelimiterSyntax to replace TagSyntax #407

Create DelimiterSyntax to replace TagSyntax #407

Conversation

chenzhiguang commented Apr 5, 2022 • edited Loading

kevmoo commented Apr 5, 2022

chenzhiguang commented Apr 5, 2022

chenzhiguang commented Apr 6, 2022

srawlins left a 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

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

chenzhiguang commented Apr 8, 2022 • edited Loading

chenzhiguang commented Apr 14, 2022

srawlins left a comment

Choose a reason for hiding this comment

chenzhiguang commented Apr 14, 2022

srawlins left a comment

Choose a reason for hiding this comment

chenzhiguang commented Apr 5, 2022 •

edited

Loading

chenzhiguang commented Apr 8, 2022 •

edited

Loading