introduce a Line class #494

chenzhiguang · 2022-11-16T08:52:16Z

The Line is used to maintain the tabRemaining for now.

This is a use case of Line, it can also explain the meaning of the tabRemaining:
https://spec.commonmark.org/0.30/#example-6

Normally the > that begins a block quote may be followed optionally by a space, which is not considered part of the content. In the following case > is followed by a tab, which is treated as if it were expanded into three spaces. Since one of these spaces is considered part of the delimiter, foo is considered to be indented six spaces inside the block quote context, so we get an indented code block starting with two spaces.

In this example the tabRemaining is the two spaces from blockquote which will be used in the nested indented code block.

coveralls · 2022-11-16T08:54:42Z

Pull Request Test Coverage Report for Build 3482854805

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

79 of 86 (91.86%) changed or added relevant lines in 18 files are covered.
2 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.3%) to 94.713%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/src/block_parser.dart	4	5	80.0%
lib/src/document.dart	2	4	50.0%
lib/src/block_syntaxes/block_syntax.dart	1	5	20.0%

Files with Coverage Reduction	New Missed Lines	%
lib/src/block_syntaxes/fenced_code_block_syntax.dart	1	98.08%
lib/src/util.dart	1	98.18%

Totals
Change from base Build 3475609872:	-0.3%
Covered Lines:	1236
Relevant Lines:	1305

💛 - Coveralls

chenzhiguang · 2022-11-16T09:11:21Z

Can we review this sooner? I need it to create PRs to fix the tab related issues.

kevmoo · 2022-11-16T19:09:42Z

We'll need to get this working w/ flutter_markdown – which is tricky because the API has changed!

chenzhiguang · 2022-11-16T19:13:38Z

Yes, I think we can use the same way as we did before to fix it.

lib/src/document.dart

chenzhiguang · 2022-11-16T20:07:32Z

Now it looks better.

lib/src/document.dart

lib/markdown.dart

CHANGELOG.md

kevmoo · 2022-11-16T20:30:20Z

Help me understand the point of Line? The API is now a bit more complex. What do we get by having a Line construct instead of just using String? (Might be good to have in the change log, too)

chenzhiguang · 2022-11-16T20:54:41Z

I found It seems the tab issues could be fixed without using this Line type. I will do more research tomorrow.

The idea came from https://github.com/tagnote-app/dart_markdown/blob/master/lib/src/line.dart, but the Line there maintains much more information.

kevmoo · 2022-11-16T20:56:21Z

Also: mind running benchmarks?

chenzhiguang · 2022-11-16T20:57:15Z

No difference, I have tried. Let me do it again

kevmoo · 2022-11-16T20:57:27Z

just because we're allocating a bit more...

chenzhiguang · 2022-11-16T21:00:16Z

Benchmark before

Benchmark after

chenzhiguang · 2022-11-16T21:03:09Z

We might need this Line in the future.

chenzhiguang · 2022-11-17T04:13:58Z

@kevmoo
We might still need Line to maintain the tabRemaining, we can not just simply convert the tabRemaining to spaces and prepend them to a string, then passes it to the nested structure.

For example: (See: https://spec.commonmark.org/0.30/#example-5)

- foo

→→bar

There are 2 space sized tabRemaining from the list block, if we just prepend two spaces to →bar like this ..→bar,
the output of the indented code block will be: (See: https://spec.commonmark.org/0.30/#example-2)

<pre><code>foo
</code></pre>

More information about tabs behaves: see https://spec.commonmark.org/0.30/#tabs

Tabs in lines are not expanded to spaces. However, in contexts where spaces help to define block structure, tabs behave as if they were replaced by spaces with a tab stop of 4 characters.

Thus, for example, a tab can be used instead of four spaces in an indented code block. (Note, however, that internal tabs are passed through as literal tabs, not expanded to spaces.)

chenzhiguang · 2022-11-17T08:31:01Z

I have added some more comment for the tabRemaining property of Line.

Sorry for this back and forth, it has been a few months since I created this Line type, I should have written a detailed comment.

chenzhiguang · 2022-11-17T19:35:45Z

Should we continue? I am blocked by this PR.

kevmoo · 2022-11-17T19:58:47Z

@srawlins should really chime in here

chenzhiguang · 2022-11-23T03:09:03Z

What do we say?

chenzhiguang · 2022-11-23T17:58:45Z

@srawlins Hi Sam, I need your decision in order to plan the next steps.
cc @kevmoo

srawlins · 2022-11-23T19:00:34Z

I will try to get to this today.

chenzhiguang · 2022-11-28T19:37:25Z

@srawlins Hi Sam, please don’t forget this one.

srawlins · 2022-11-29T16:29:34Z

lib/src/line.dart

+  // A line containing no characters, or a line containing only spaces
+  // (`U+0020`) or tabs (`U+0009`), is called a blank line.
+  // https://spec.commonmark.org/0.30/#blank-line
+  bool get isBlankLine => emptyPattern.hasMatch(content);


It'd be interesting to consider how many Lines are queried for whether they are blank. I suspect it is a high number, but it'd be good to really look. And how many Lines are queried multiple times whether they are blank. If most isBlankLine is called on most Lines, and called multiple times on some Lines, it might be a win to store this bool as a final field.

Fixed. The ListSyntax does call isBlankLine on most lines and also calls multiple times on some lines

srawlins · 2022-11-29T16:31:37Z

lib/src/line.dart

+    this.tabRemaining,
+  });
+
+  Map<String, dynamic> toMap() => {


If this is only called in tests, I don't think it is worth it to implement here.

test/util_test.dart

srawlins

Super cool, great feature, sorry for the delay.

chenzhiguang · 2022-11-29T19:04:21Z

No worries! Thanks!

* introduce a Line class * Update CHANGELOG * maintain backwards compatibility * Update CHANGELOG * Improve CHANGELOG.md * Improve CHANGELOG.md * Add more comment for tabRemaining * Remove toMap from Line * set `isBlankLine` of `Line` as final * remove extension name LineX

kevmoo reviewed Nov 16, 2022

View reviewed changes

lib/src/document.dart Outdated Show resolved Hide resolved

chenzhiguang force-pushed the introduce-a-Line-class branch 2 times, most recently from f28dfd8 to 6d0c977 Compare November 16, 2022 20:05

kevmoo reviewed Nov 16, 2022

View reviewed changes

lib/src/document.dart Show resolved Hide resolved

lib/markdown.dart Show resolved Hide resolved

kevmoo reviewed Nov 16, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

kevmoo reviewed Nov 16, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

chenzhiguang closed this Nov 17, 2022

chenzhiguang reopened this Nov 17, 2022

chenzhiguang added 3 commits November 24, 2022 20:02

introduce a Line class

f8becc5

Update CHANGELOG

02a13d9

maintain backwards compatibility

8a53224

chenzhiguang added 4 commits November 24, 2022 20:02

Update CHANGELOG

13bcf19

Improve CHANGELOG.md

857f099

Improve CHANGELOG.md

b9aee9c

Add more comment for tabRemaining

3c87feb

chenzhiguang force-pushed the introduce-a-Line-class branch from 6bfbb75 to 3c87feb Compare November 24, 2022 19:02

srawlins reviewed Nov 29, 2022

View reviewed changes

Remove toMap from Line

cf1e92a

kevmoo reviewed Nov 29, 2022

View reviewed changes

test/util_test.dart Outdated Show resolved Hide resolved

set isBlankLine of Line as final

fc4e498

chenzhiguang force-pushed the introduce-a-Line-class branch from 43653d3 to fc4e498 Compare November 29, 2022 18:53

remove extension name LineX

af1376b

srawlins approved these changes Nov 29, 2022

View reviewed changes

srawlins merged commit c3d1136 into dart-archive:master Nov 29, 2022

chenzhiguang deleted the introduce-a-Line-class branch November 29, 2022 19:09

introduce a Line class #494

introduce a Line class #494

Uh oh!

Conversation

chenzhiguang commented Nov 16, 2022

Uh oh!

coveralls commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 3482854805

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

chenzhiguang commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevmoo commented Nov 16, 2022

Uh oh!

chenzhiguang commented Nov 16, 2022

Uh oh!

Uh oh!

chenzhiguang commented Nov 16, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevmoo commented Nov 16, 2022

Uh oh!

chenzhiguang commented Nov 16, 2022

Uh oh!

kevmoo commented Nov 16, 2022

Uh oh!

chenzhiguang commented Nov 16, 2022

Uh oh!

kevmoo commented Nov 16, 2022

Uh oh!

chenzhiguang commented Nov 16, 2022

Benchmark before

Benchmark after

Uh oh!

chenzhiguang commented Nov 16, 2022

Uh oh!

chenzhiguang commented Nov 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chenzhiguang commented Nov 17, 2022

Uh oh!

chenzhiguang commented Nov 17, 2022

Uh oh!

kevmoo commented Nov 17, 2022

Uh oh!

chenzhiguang commented Nov 23, 2022

Uh oh!

chenzhiguang commented Nov 23, 2022

Uh oh!

srawlins commented Nov 23, 2022

Uh oh!

chenzhiguang commented Nov 28, 2022

Uh oh!

srawlins Nov 29, 2022

Choose a reason for hiding this comment

Uh oh!

chenzhiguang Nov 29, 2022

Choose a reason for hiding this comment

Uh oh!

srawlins Nov 29, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

srawlins left a comment

Choose a reason for hiding this comment

Uh oh!

chenzhiguang commented Nov 29, 2022

Uh oh!

Uh oh!

coveralls commented Nov 16, 2022 •

edited

Loading

chenzhiguang commented Nov 16, 2022 •

edited

Loading

chenzhiguang commented Nov 17, 2022 •

edited

Loading