Skip to content
This repository was archived by the owner on Feb 24, 2025. It is now read-only.

Improve FencedCodeBlockSyntax #478

Merged
merged 4 commits into from
Nov 8, 2022
Merged

Improve FencedCodeBlockSyntax #478

merged 4 commits into from
Nov 8, 2022

Conversation

chenzhiguang
Copy link
Contributor

Strictly improves the test coverage.

@coveralls
Copy link

coveralls commented Oct 28, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 3346469099

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.

Details

  • 45 of 45 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.07%) to 95.043%
Totals Coverage Status
Change from base Build 3323865806: 0.07%
Covered Lines: 1227
Relevant Lines: 1291
💛 - Coveralls

@chenzhiguang chenzhiguang mentioned this pull request Oct 28, 2022
Merged
@chenzhiguang chenzhiguang force-pushed the Improve-FencedCodeBlockSyntax branch 3 times, most recently from 893c9b8 to 7c63754 Compare October 31, 2022 16:57
@kevmoo
Copy link
Contributor

kevmoo commented Nov 4, 2022

@chenzhiguang – please rebase on master – we now have flutter_markdown validation in place

If there are breaks there, we'll have to dig to see what's expected/intended.

@chenzhiguang chenzhiguang force-pushed the Improve-FencedCodeBlockSyntax branch from 4129730 to 5f5a2ad Compare November 4, 2022 05:48
@chenzhiguang
Copy link
Contributor Author

Rebased

@chenzhiguang
Copy link
Contributor Author

Can we have a look at this one? It is important to my next PR.

srawlins
srawlins suggested changes Nov 4, 2022
View reviewed changes
Copy link
Collaborator

@srawlins srawlins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this all looks really good.

Would you mind posting benchmarks, before and after? I get ancy with RegExp changes. There are definitely strings that have really bad performance with some RegExp, and this has appeared in Flutter code before.

Just a few other requests.

@chenzhiguang
Copy link
Contributor Author

Benchmark

Before

Screenshot 2022-11-04 at 18 01 48

After

Screenshot 2022-11-04 at 18 05 01

@kevmoo
Copy link
Contributor

kevmoo commented Nov 4, 2022

@srawlins – I think benchmarks look fine, no?

@chenzhiguang chenzhiguang force-pushed the Improve-FencedCodeBlockSyntax branch from 845f6d8 to 664d9a0 Compare November 8, 2022 03:18
@chenzhiguang
Copy link
Contributor Author

rebased

srawlins
srawlins approved these changes Nov 8, 2022
View reviewed changes
Copy link
Collaborator

@srawlins srawlins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@srawlins
Copy link
Collaborator

srawlins commented Nov 8, 2022

CC @devoncarew

This one passes flutter_markdown 🎉

@srawlins srawlins merged commit 0a37f46 into dart-archive:master Nov 8, 2022
@chenzhiguang
Copy link
Contributor Author

Cool, thanks!

@chenzhiguang chenzhiguang deleted the Improve-FencedCodeBlockSyntax branch November 8, 2022 16:44
mosuem pushed a commit to dart-lang/tools that referenced this pull request Dec 9, 2024
@chenzhiguang
Improve FencedCodeBlockSyntax (dart-archive/markdown#478)
31228ff 
* Improve FencedCodeBlockSyntax

* Update gfm_stats.txt

* Fix some requests

* run tool/stats.dart --update-files
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@srawlins srawlins srawlins approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chenzhiguang @coveralls @kevmoo @srawlins