-
Notifications
You must be signed in to change notification settings - Fork 249
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
upgrade markdown package reference #2364
Changes from 4 commits
6050484
9084683
fddd6dc
95ea5b2
7578b41
a5ae6c2
a1d18d8
5f428bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,9 +23,12 @@ MutableDocument deserializeMarkdownToDocument( | |
List<ElementToNodeConverter> customElementToNodeConverters = const [], | ||
bool encodeHtml = false, | ||
}) { | ||
final markdownLines = const LineSplitter().convert(markdown); | ||
final markdownLines = const LineSplitter().convert(markdown).map((l) { | ||
return md.Line(l); | ||
}).toList(); | ||
|
||
final markdownDoc = md.Document( | ||
encodeHtml: encodeHtml, | ||
blockSyntaxes: [ | ||
...customBlockSyntax, | ||
if (syntax == MarkdownSyntax.superEditor) ...[ | ||
|
@@ -519,21 +522,28 @@ abstract class ElementToNodeConverter { | |
DocumentNode? handleElement(md.Element element); | ||
} | ||
|
||
/// A Markdown [TagSyntax] that matches underline spans of text, which are represented in | ||
/// A Markdown [DelimiterSyntax] that matches underline spans of text, which are represented in | ||
/// Markdown with surrounding `¬` tags, e.g., "this is ¬underline¬ text". | ||
/// | ||
/// This [TagSyntax] produces `Element`s with a `u` tag. | ||
class UnderlineSyntax extends md.TagSyntax { | ||
UnderlineSyntax() : super('¬', requiresDelimiterRun: true, allowIntraWord: true); | ||
/// This [DelimiterSyntax] produces `Element`s with a `u` tag. | ||
class UnderlineSyntax extends md.DelimiterSyntax { | ||
|
||
/// This is required by md.DelimiterSyntax, in theory the constructor takes an optional tags member, but a bug in | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment still doesn't tell us what this thing is. What does "u" mean? What's a Assume the next person who's responsible for this has very little understanding of the inside of the markdown package. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was trying to explain that it's not actually supposed to be required from the class definition: https://pub.dev/documentation/markdown/latest/markdown/DelimiterSyntax-class.html The constructor takes a nullable. However, the problem is there is a bug in the underlying dart library if you don't pass it. Due to these two lines, one sets it to const [] if not passed, then the next tries to sort. So we have to pass something or it blows up. That's the only reason it's there. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand that point about the bug in the package - but those questions I raised above are still real questions that a reader will have. Can you please address those in the code comment? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added this explanation to the code, hope it clarifies the reasoning this is required better. |
||
/// the Markdown lib replaces that with a const [], and then tries to sort it. | ||
static final _tags = [ md.DelimiterTag("u", 1) ]; | ||
|
||
UnderlineSyntax() : super('¬', requiresDelimiterRun: true, allowIntraWord: true, tags: _tags); | ||
|
||
@override | ||
md.Node? close( | ||
Iterable<md.Node>? close( | ||
md.InlineParser parser, | ||
md.Delimiter opener, | ||
md.Delimiter closer, { | ||
required List<md.Node> Function() getChildren, | ||
required String tag, | ||
}) { | ||
return md.Element('u', getChildren()); | ||
final element = md.Element('u', getChildren()); | ||
return [ element ]; | ||
} | ||
} | ||
|
||
|
@@ -548,7 +558,7 @@ class _ParagraphWithAlignmentSyntax extends _EmptyLinePreservingParagraphSyntax | |
|
||
@override | ||
bool canParse(md.BlockParser parser) { | ||
if (!_alignmentNotationPattern.hasMatch(parser.current)) { | ||
if (!_alignmentNotationPattern.hasMatch(parser.current.content)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was this a pre-existing error on our part? Should we have been accessing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Markdown lib changed from String to Line which has a content member, so this just redirects to where the string is now. |
||
return false; | ||
} | ||
|
||
|
@@ -564,7 +574,7 @@ class _ParagraphWithAlignmentSyntax extends _EmptyLinePreservingParagraphSyntax | |
/// We found a paragraph alignment token, but the block after the alignment token isn't a paragraph. | ||
/// Therefore, the paragraph alignment token is actually regular content. This parser doesn't need to | ||
/// take any action. | ||
if (_standardNonParagraphBlockSyntaxes.any((syntax) => syntax.pattern.hasMatch(nextLine))) { | ||
if (_standardNonParagraphBlockSyntaxes.any((syntax) => syntax.pattern.hasMatch(nextLine.content))) { | ||
return false; | ||
} | ||
|
||
|
@@ -575,7 +585,7 @@ class _ParagraphWithAlignmentSyntax extends _EmptyLinePreservingParagraphSyntax | |
|
||
@override | ||
md.Node? parse(md.BlockParser parser) { | ||
final match = _alignmentNotationPattern.firstMatch(parser.current); | ||
final match = _alignmentNotationPattern.firstMatch(parser.current.content); | ||
|
||
// We've parsed the alignment token on the current line. We know a paragraph starts on the | ||
// next line. Move the parser to the next line so that we can parse the paragraph. | ||
|
@@ -630,13 +640,13 @@ class _EmptyLinePreservingParagraphSyntax extends md.BlockSyntax { | |
return false; | ||
} | ||
|
||
if (parser.current.isEmpty) { | ||
if (parser.current.content.isEmpty) { | ||
// We consider this input to be a separator between blocks because | ||
// it started with an empty line. We want to parse this input. | ||
return true; | ||
} | ||
|
||
if (_isAtParagraphEnd(parser, ignoreEmptyBlocks: _endsWithHardLineBreak(parser.current))) { | ||
if (_isAtParagraphEnd(parser, ignoreEmptyBlocks: _endsWithHardLineBreak(parser.current.content))) { | ||
// Another parser wants to parse this input. Let the other parser run. | ||
return false; | ||
} | ||
|
@@ -648,12 +658,12 @@ class _EmptyLinePreservingParagraphSyntax extends md.BlockSyntax { | |
@override | ||
md.Node? parse(md.BlockParser parser) { | ||
final childLines = <String>[]; | ||
final startsWithEmptyLine = parser.current.isEmpty; | ||
final startsWithEmptyLine = parser.current.content.isEmpty; | ||
|
||
// A hard line break causes the next line to be treated | ||
// as part of the same paragraph, except if the next line is | ||
// the beginning of another block element. | ||
bool hasHardLineBreak = _endsWithHardLineBreak(parser.current); | ||
bool hasHardLineBreak = _endsWithHardLineBreak(parser.current.content); | ||
|
||
if (startsWithEmptyLine) { | ||
// The parser started at an empty line. | ||
|
@@ -669,7 +679,7 @@ class _EmptyLinePreservingParagraphSyntax extends md.BlockSyntax { | |
return null; | ||
} | ||
|
||
if (!_blankLinePattern.hasMatch(parser.current)) { | ||
if (!_blankLinePattern.hasMatch(parser.current.content)) { | ||
// We found an empty line, but the following line isn't blank. | ||
// As there is no hard line break, the first line is consumed | ||
// as a separator between blocks. | ||
|
@@ -682,7 +692,7 @@ class _EmptyLinePreservingParagraphSyntax extends md.BlockSyntax { | |
childLines.add(''); | ||
|
||
// Check for a hard line break, so we consume the next line if we found one. | ||
hasHardLineBreak = _endsWithHardLineBreak(parser.current); | ||
hasHardLineBreak = _endsWithHardLineBreak(parser.current.content); | ||
parser.advance(); | ||
} | ||
|
||
|
@@ -691,9 +701,9 @@ class _EmptyLinePreservingParagraphSyntax extends md.BlockSyntax { | |
// ends with a hard line break. | ||
while (!_isAtParagraphEnd(parser, ignoreEmptyBlocks: hasHardLineBreak)) { | ||
final currentLine = parser.current; | ||
childLines.add(currentLine); | ||
childLines.add(currentLine.content); | ||
|
||
hasHardLineBreak = _endsWithHardLineBreak(currentLine); | ||
hasHardLineBreak = _endsWithHardLineBreak(currentLine.content); | ||
|
||
parser.advance(); | ||
} | ||
|
@@ -777,7 +787,7 @@ class _TaskSyntax extends md.BlockSyntax { | |
|
||
@override | ||
md.Node? parse(md.BlockParser parser) { | ||
final match = pattern.firstMatch(parser.current); | ||
final match = pattern.firstMatch(parser.current.content); | ||
if (match == null) { | ||
return null; | ||
} | ||
|
@@ -795,10 +805,10 @@ class _TaskSyntax extends md.BlockSyntax { | |
// - find a blank line OR | ||
// - find the start of another block element (including another task) | ||
while (!parser.isDone && | ||
!_blankLinePattern.hasMatch(parser.current) && | ||
!_standardNonParagraphBlockSyntaxes.any((syntax) => syntax.pattern.hasMatch(parser.current))) { | ||
!_blankLinePattern.hasMatch(parser.current.content) && | ||
!_standardNonParagraphBlockSyntaxes.any((syntax) => syntax.pattern.hasMatch(parser.current.content))) { | ||
buffer.write('\n'); | ||
buffer.write(parser.current); | ||
buffer.write(parser.current.content); | ||
|
||
parser.advance(); | ||
} | ||
|
@@ -832,7 +842,7 @@ class _HeaderWithAlignmentSyntax extends md.BlockSyntax { | |
|
||
@override | ||
bool canParse(md.BlockParser parser) { | ||
if (!_alignmentNotationPattern.hasMatch(parser.current)) { | ||
if (!_alignmentNotationPattern.hasMatch(parser.current.content)) { | ||
return false; | ||
} | ||
|
||
|
@@ -846,7 +856,7 @@ class _HeaderWithAlignmentSyntax extends md.BlockSyntax { | |
} | ||
|
||
// Only parse if the next line is header. | ||
if (!_headerSyntax.pattern.hasMatch(nextLine)) { | ||
if (!_headerSyntax.pattern.hasMatch(nextLine.content)) { | ||
return false; | ||
} | ||
|
||
|
@@ -855,7 +865,7 @@ class _HeaderWithAlignmentSyntax extends md.BlockSyntax { | |
|
||
@override | ||
md.Node? parse(md.BlockParser parser) { | ||
final match = _alignmentNotationPattern.firstMatch(parser.current); | ||
final match = _alignmentNotationPattern.firstMatch(parser.current.content); | ||
|
||
// We've parsed the alignment token on the current line. We know a header starts on the | ||
// next line. Move the parser to the next line so that we can parse the header. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1129,17 +1129,21 @@ This is some code | |
expect((document.first as ListItemNode).text.text, isEmpty); | ||
}); | ||
|
||
test('unordered list followd by empty list item', () { | ||
const markdown = """- list item 1 | ||
- """; | ||
test('unordered list followed by empty list item', () { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a change in behavior here in this specific case introduced by the underlying parser changing, but was the intent of the test really to verify that the hanging - was deleted from the line in a weird case that markdown parsers don't seem to agree on? CommonMark spec seems to say that empty list items should be allowed, not stripped. This updates the test to verify that empty list items should be kept like the spec says, rather than test for something that seems to be up interpreted differently depending on the parser being used. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like this is where we introduced this test: #1822 |
||
const markdown = """ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this leading newline need to be here? If a Markdown blob begins with a list item, it won't have a leading newline. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't need to be there, will remove the leading. |
||
- list item 1 | ||
- """; | ||
|
||
final document = deserializeMarkdownToDocument(markdown); | ||
|
||
expect(document.nodeCount, 1); | ||
expect(document.nodeCount, 2); | ||
|
||
expect(document.getNodeAt(0)!, isA<ListItemNode>()); | ||
expect((document.getNodeAt(0)! as ListItemNode).type, ListItemType.unordered); | ||
expect((document.getNodeAt(0)! as ListItemNode).text.text, 'list item 1'); | ||
expect(document.getNodeAt(1)!, isA<ListItemNode>()); | ||
expect((document.getNodeAt(1)! as ListItemNode).type, ListItemType.unordered); | ||
expect((document.getNodeAt(1)! as ListItemNode).text.text, ''); | ||
}); | ||
|
||
test('parses mixed unordered and ordered items', () { | ||
|
@@ -1433,10 +1437,22 @@ with multiple lines | |
expect(document.getNodeAt(25)!, isA<ParagraphNode>()); | ||
}); | ||
|
||
test('paragraph with strikethrough', () { | ||
test('paragraph with single ~ should not be strikethrough', () { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a change in behavior, but it seems to be more correct. The underlying dart markdown parser seems to be more strict now, but it does seem to align better with the generally accepted pattern for strikethrough. Should the markdown parser accept the decision of the underlying markdown lib to change the behavior here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the new name of this test is ambiguous. When I read it, I thought it meant a single standalone " If you're saying that the new package version doesn't parse " Adding support for double "~~" should be fine because its a new feature, and if that really is the norm, then I don't foresee complaints. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @matthew-carroll added single line strikethrough parsing node, a bit hacky to get it to work since underlying markdown parser doesn't like having two sort of similar nodes, but does work as long as the order in the list is correct. Added comments to explain. |
||
final doc = deserializeMarkdownToDocument('~This is~ a paragraph.'); | ||
final styledText = (doc.getNodeAt(0)! as ParagraphNode).text; | ||
|
||
// Ensure text within the range isn't attributed. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this test be reverted, given that you added the single-character parser? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. Test was reverted but comment wasn't. |
||
expect(styledText.getAllAttributionsAt(0).contains(strikethroughAttribution), false); | ||
expect(styledText.getAllAttributionsAt(6).contains(strikethroughAttribution), false); | ||
|
||
// Ensure text outside the range isn't attributed. | ||
expect(styledText.getAllAttributionsAt(7).contains(strikethroughAttribution), false); | ||
}); | ||
|
||
test('paragraph with double strikethrough', () { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a test for this case since it was missing |
||
final doc = deserializeMarkdownToDocument('~~This is~~ a paragraph.'); | ||
final styledText = (doc.getNodeAt(0)! as ParagraphNode).text; | ||
|
||
// Ensure text within the range is attributed. | ||
expect(styledText.getAllAttributionsAt(0).contains(strikethroughAttribution), true); | ||
expect(styledText.getAllAttributionsAt(6).contains(strikethroughAttribution), true); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happened to require this difference? I'm not sure what the existing code did, but it's not immediately obvious what the difference is in impact between the original code and this new code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Markdown lib changed from taking
List<String>
to takingList<Line>
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add an explicit type parameter to your map function so that the difference is unambiguous? Part of the confusion here is that reading the code doesn't tell us what type "l" is.