Skip to content
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

Do not allow reference link labels contain left brackets #402

Merged
merged 2 commits into from
Mar 30, 2022
Merged

Do not allow reference link labels contain left brackets #402

merged 2 commits into from
Mar 30, 2022

Conversation

chenzhiguang
Copy link
Contributor

Fix: #335

@srawlins
Copy link
Member

Does this fix a Commonmark spec then? Can you run tool/stats.dart --update-files and tool/stats.dart --update-files --flavor=gfm to update the compliance stat files?

dart tool/stats.dart --update-files
@chenzhiguang
Copy link
Contributor Author

Does this fix a Commonmark spec then? Can you run tool/stats.dart --update-files and tool/stats.dart --update-files --flavor=gfm to update the compliance stat files?

I have updated the stat files by running stats.dart

@srawlins
Copy link
Member

We should not change the unit files case-by-case, they are meant to be an exact set of test cases from a single version of the commonmark specs. I'll look into bumping them.

@srawlins
Copy link
Member

Sorry, I totally mis-remembered how we maintain the spec tests. @kevmoo can maybe confirm:

tool/stats.dart --update-files will use tool/common_mark_tests.json to run specs, and update all of the unit files in test/.

Then the unit files represent what the markdown package currently outputs, so that a clean checkout will always have 100% tests passing.

tool/common_mark_tests.json is updated separately (download?), e.g. in #383.

When someone improves compliance (like this PR), the unit file changes, and tool/*_stats.json and tool/*_stats.txt are updated (via tool/stats.dart --update-files).

@kevmoo
Copy link
Member

kevmoo commented Mar 30, 2022

I'd have to re-learn, too – even though I wrote it! That sounds right, @srawlins

@kevmoo
Copy link
Member

kevmoo commented Mar 30, 2022

Looks like the output files change, but the stats don't change – so equally broken it seems?

@kevmoo
Copy link
Member

kevmoo commented Mar 30, 2022

So this change seems to make things less broken – but it's still broken.

<p>[foo][ref[]</p>
<p>[ref[]: /uri</p>

According to spec, this is what we should see.

Copy link
Member

@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'll take the PR because it is strictly better than what we have today 😁. To pass commonmark 545, we just need to also change parsing of the link reference (in addition to the reference link, which you solved).

Thanks!

@srawlins srawlins merged commit 0bac4f6 into dart-lang:master Mar 30, 2022
@chenzhiguang chenzhiguang deleted the Fix-reference-link-label branch April 9, 2022 12:17
roubachof pushed a commit to roubachof/markdown that referenced this pull request Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reference Link Labels Cannot Contain Brackets
3 participants