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

Add support for wikilinks format #407

Merged
merged 10 commits into from
May 16, 2024
Merged

Add support for wikilinks format #407

merged 10 commits into from
May 16, 2024

Conversation

digitalmoksha
Copy link
Collaborator

@digitalmoksha digitalmoksha commented May 9, 2024

Add extensions to support both styles of wikilinks

  • [[display text|target page]] [GitHub style]
  • [[target page|display text]] [regular wiki link]

most likely with wikilinks_title_before_pipe and wikilinks_title_after_pipe extension names.

Inspired by commonmark-hs

Copy link
Contributor

github-actions bot commented May 9, 2024

Command Mean [ms] Min [ms] Max [ms] Relative
./bench.sh ./comrak-036be5f 320.0 ± 3.0 314.4 326.9 2.87 ± 0.03
./bench.sh ./comrak-main 319.8 ± 5.9 317.5 347.5 2.87 ± 0.06
./bench.sh ./pulldown-cmark 111.4 ± 0.8 109.6 113.6 1.00
./bench.sh ./cmark-gfm 118.3 ± 1.4 117.1 124.1 1.06 ± 0.02
./bench.sh ./markdown-it 483.0 ± 3.8 479.2 492.4 4.34 ± 0.05

Run on Thu May 9 23:01:12 UTC 2024

@digitalmoksha digitalmoksha force-pushed the bw-wikilinks branch 3 times, most recently from 5c8d06a to 6828974 Compare May 10, 2024 21:50
supporting either

[[url|link text]]

or

[[link text|url]]
@digitalmoksha
Copy link
Collaborator Author

This works, but I hooked it before the normal bracket link processing cause it turned out to be much easier/cleaner, but I now realize it bypasses the normal inline processing of the link text. And I kinda think the link text should behave the same between the two. Which means emphasis, etc, would get properly handled in the link text, as it does for normal markdown links.

But maybe that's not expected in wikilinks. It doesn't work that way in commonmark-hs

cabal run exes -- -x wikilinks_title_before_pipe

[[_abc_|def]]

yields

<p><a href="def" title="wikilink">_abc_</a></p>

@digitalmoksha
Copy link
Collaborator Author

digitalmoksha commented May 12, 2024

Hmm, I need to spin up a wiki on GitHub and see what their behavior is.

EDIT: which would require me to upgrade :-(

@digitalmoksha
Copy link
Collaborator Author

GH doesn't support markdown in the link text either. In fact GH doesn't recognize [[_abc_|def]] as a valid wikilink, which I think might be a bug.

So I would say the approach right now is good enough.

Still need to fix a failing spec.

@digitalmoksha digitalmoksha force-pushed the bw-wikilinks branch 2 times, most recently from 0304408 to 9b3896b Compare May 12, 2024 19:49
This is so it’s possible to tell the difference between
a normal link and a wikilink during post-processing.
@digitalmoksha
Copy link
Collaborator Author

@kivikakk I think this is ready to go. Mind taking a look at it when you have some time?

Copy link
Owner

@kivikakk kivikakk left a comment

Choose a reason for hiding this comment

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

This looks really good! I love the extensive testing — I don't have much mind myself at the moment for thoroughly evaluating the correctness of the inline parsing bits, but I'm hoping that between tests, fuzzing, and any other reviews, it should be fine.

src/nodes.rs Outdated Show resolved Hide resolved
src/nodes.rs Outdated Show resolved Hide resolved
src/parser/inlines.rs Outdated Show resolved Hide resolved
src/parser/inlines.rs Outdated Show resolved Hide resolved
@digitalmoksha
Copy link
Collaborator Author

Will be running cargo +nightly fuzz run quadratic -j 6 all night

@digitalmoksha
Copy link
Collaborator Author

==85446== libFuzzer: run interrupted; exiting
stat::number_of_executed_units: 64005
stat::average_exec_per_sec:     243
stat::new_units_added:          3258
stat::slowest_unit_time_sec:    0
stat::peak_rss_mb:              550
INFO: exiting: 18432 time: 135145s

@digitalmoksha
Copy link
Collaborator Author

I think this is tightened up now. Even added a test to show how it works and doesn't work in pipe tables. Which is how it currently works in commonmark-hs, GitHub, and GitLab.

@kivikakk
Copy link
Owner

This looks great. I've made a small adjustment in b769ee4 such that the Text node within the wikilink has the sourcepos of the component that produced the actual text (i.e. the label part if one exists, otherwise the URL). This all looks good to me — thank you! 🎉

@kivikakk kivikakk enabled auto-merge May 16, 2024 10:02
@kivikakk kivikakk merged commit aa79cac into main May 16, 2024
39 checks passed
@kivikakk kivikakk deleted the bw-wikilinks branch May 16, 2024 10:04
@digitalmoksha
Copy link
Collaborator Author

the Text node within the wikilink has the sourcepos of the component that produced the actual text

Good call, thanks!

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.

2 participants