search

markdown-it-rust / markdown-it

markdown-it js library rewritten in rust
Other
79 stars 9 forks source link

Smartquotes #5

Closed black-puppydog closed 1 year ago

black-puppydog commented 1 year ago

This is the follow-up to #4 that I mentioned before, so it includes all of those commits, plus "a few" more.

I should first say thanks again for reference implementation in JS, wouldn't have been able to do it without. 🙂

Of course this still needs linting and squashing, and I haven't run it through benchmarking at all. But at least I managed to avoid making string copies all over the place, so that's nice. I had to add some lifetime annotations to the walk function to do that. Is that okay, or do you see a more elegant way to do this?

If you do want to crawl through the unsquashed history here you'll find that the IT WORKS commit reads a lot like the JS version. However, I found that to be too long, so I started breaking things out, and the result is actually fairly readable, if I say so myself. 😛

black-puppydog commented 1 year ago

didn't check smartquotes.rs implementation yet, typographer implementation looks good (maybe merge it separately)

Indeed, I left the typographer PR #4 open because I figured it's kinda ready, while this one here definitely needs "some" cleaning. But this PR really depends on the typographer as well, so I based the smartquotes branch off off the typographer one.

Thank you so much for the feedback! I'll go over it when I'm a little less tired. Some of it might also go into #4 then, we'll see. :)

black-puppydog commented 1 year ago

Integrated the first batch of feedback, some still to do.

This still includes the commits from #4 but everything starting from Generate smartquote test cases (8947b044b2eec1a84543022dea957087c2c120ac) belongs to this PR.

black-puppydog commented 1 year ago

Okay, I went over most of the feedback now.

I also went ahead and added the two modules (typographer and smartquotes) to the extras module by default. Together with an example. I used a second call just to keep the lines in the example short.

black-puppydog commented 1 year ago

I have to say I don't actually understand what the deps CI checks for. I did run the same commands locally at some point and it failed in the same way. I can try to fix this, but not sure what I'm really looking for.

rlidwka commented 1 year ago

I have to say I don't actually understand what the deps CI checks for.

It checks with lowest possible versions of dependencies to make sure lower semver bounds are correct.

https://github.com/rust-lang/cargo/issues/5657

In short: forget about it, probably regex crate version needs to get bumped.

Is it good to merge as is, or did you want to add anything else? I'll merge if it is, and fix tests on my own.

black-puppydog commented 1 year ago

For me it's good as is. Looking forward to seeing what the fix ends up being :)

rlidwka commented 1 year ago

Looking forward to seeing what the fix ends up being :)

Old code compiled fine with regex 0.2.0, new code doesn't, that's all. - https://github.com/rlidwka/markdown-it.rs/commit/741dcd406693caa9109770589fd8a39d607f91fd

Also CLI part was forgotten in PR above, so added it. - https://github.com/rlidwka/markdown-it.rs/commit/eb5459039685d19cefd0361859422118d08d35d4

Seems to be working, gonna release it as 0.5.0 sometime soon. Thanks!

black-puppydog commented 1 year ago

Ah damn, I have never needed the CLI so yeah I totally forgot about that.