Typographer (Replacing ©, ®, ™, ... etc)

[black-puppydog]

This is a straight-up re-implementation of lib/rules_core/replacements.js from the JS library.

There are still a few corner cases I'm not sure about, but this is progressed enough that I'd like feedback if anyone is willing to give. 🙂

I'm especially unsure about the linkify.rs modification. I needed this because the JS implementation checks for auto-linkified links, and doesn't touch the text in those. I tried multiple ways to do this.

The "easiest" would have been to be able to walk up the tree of nodes, to determine whether one of my parents is Linkified. We don't have that though, right?

The next attempt was to modify walk_mut so that I could pass a second closure that would be called when exiting the node. That way I could keep track of the linkification status, similar to how the JS lib does it. However, I never managed to get this past the borrow checker, because I was using a single linkification_level: i32 which I needed to mut-pass to both the entry and exit closure. It might well be that there would be a trivial fix for this (I'm still quite new to Rust and my stdlib-foo is not great) but in any case that would mean a bunch of boiler-plate to insert into all four walk[_post][_mut] methods and everywhere they're used.

So my solution is to attach an empty LinkifyMarker struct to the nodes when they get auto-linkified. I then just check if the text node I'm dealing with has that marker, and ignore it if it's the case.

Side note: the Error I'm encountering in #3 is trivial to fix with this branch: just add the typographer plugin to the parser during test, and it seems work!

To do

• Documentation
• Decide on where/whether this should be included in any default values in the crate

[black-puppydog]

Okay after adding another test (fixture) and cross-checking that one with the JS implementation as well, I'm now reasonably confident that this works as intended. :)

[rlidwka]

in markdown-it here \n is the only possible newline, other newlines will get normalized for simplicity

so I don't think any problem with newlines exists

[black-puppydog]

I just checked this by inserting this test into markdown-it-typographer.rs:

#[test]
fn can_handle_unicode_line_tabulation() {
    run("--\u{000B}---", "<p>– —</p>");
}

This uses the line tabulation character which println! does interpret as a newline (but preserving the horizontal indentation as expected).

The typography works fine, it produces "<p>–\u{b}—</p>\n" and the same happens in the JS lib (I checked). Other newlines like \u{0085} aka next line get completely ignored by JS, while the rust version would produce \u{85} in the output.

So for the typography I will actually remove the comment since it works fine (matching these more exotic newlines works just as fine as matching $ or ^) but I'm not sure what you mean by "other newlines will get normalized". Is that something that is planned, or a missing feature, or should it already work and it' somehow broken in these tests?

[rlidwka]

Case-insensitive flag in Rust looks something like this:
Regex::new(r"(?i)\((c|tm|r)\)").unwrap()

In your version, (Tm) will not get replaced, which differs from JS.

But I guess that was intentional to simplify replace_abbreviation below?

[black-puppydog]

That was actually not intentional; I can add a test for that, and thanks for the pointer towards (?i)! :)

[rlidwka]

The "easiest" would have been to be able to walk up the tree of nodes, to determine whether one of my parents is Linkified. We don't have that though, right?

In theory, custom walk function will do the job (i.e. not using walk_mut, but writing your own). Not ideal, I know.

So my solution is to attach an empty LinkifyMarker struct to the nodes when they get auto-linkified. I then just check if the text node I'm dealing with has that marker, and ignore it if it's the case.

Actually, as far as I remember, text nodes that shouldn't be replaced should have TextSpecial type instead of Text (which is not done in JS for historical reasons). I'll look into fixing that.

Decide on where/whether this should be included in any default values in the crate

People usually use replacements together with smartquotes. Since smartquotes aren't implemented, I don't think this feature is complete, thus don't enable it by default yet.

[rlidwka]

I'm especially unsure about the linkify.rs modification. I needed this because the JS implementation checks for auto-linkified links, and doesn't touch the text in those. I tried multiple ways to do this.

I believe this is the best way to fix it: 547505e

[codecov-commenter]

Codecov Report

Base: 95.13% // Head: 95.32% // Increases project coverage by +0.19% 🎉

Coverage data is based on head (547505e) compared to base (c9b087a).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 547505e differs from pull request most recent head 7fa1847. Consider uploading reports for the commit 7fa1847 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master       #4      +/-   ##
==========================================
+ Coverage   95.13%   95.32%   +0.19%     
==========================================
  Files          58       54       -4     
  Lines        2302     2226      -76     
==========================================
- Hits         2190     2122      -68     
+ Misses        112      104       -8     

Impacted Files	Coverage Δ
src/plugins/cmark/inline/autolink.rs	100.00% <100.00%> (ø)
src/plugins/extra/linkify.rs	96.72% <100.00%> (+0.11%)	⬆️
examples/ferris/main.rs
examples/ferris/inline_rule.rs
examples/ferris/block_rule.rs
examples/ferris/core_rule.rs

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[black-puppydog]

Thanks very much for the feedback, I'll look into it once I find a few quiet minutes. :)

People usually use replacements together with smartquotes. Since smartquotes aren't implemented, I don't think this feature is complete

Indeed, that's a good point. I'm already looking into porting the smartquotes next, although the algorithm used in JS is annoying to implement in rust due to borrow checking. But I'm sure I'll get there :D

[black-puppydog]

This became quite a comprehensive rebase so that lazy_static doesn't get used anywhere in the commit history. Thanks to your commit, the whole linkify modification seems to have become unnecessary. Yay for simplicity! 🎉

The linebreak question is a separate issue, if any.
I also added a bit of documentation, especially also noting that this is not enabled with extras by default.

[black-puppydog]

Just found time to add one last thing here; the way I had used once_cell was the "simple but verbose" way. I just added a commit that takes advantage of once_cell::Lazy which simplifies the code a bit.

[black-puppydog]

Following the comments in #5 I went ahead and made the changes you indicated. Thanks again for the feedback, this is a great learning experience for me! I appreciate that it takes a lot of time to do these reviews.

✅ unreachable instead of panic
✅ Cow and if let instead of to_string and while
✅ closure instead of impl Replacer