Callback for Broken Reference Links #100
Conversation
kivikakk
left a comment
kivikakk
left a comment
There was a problem hiding this comment.
I love this! Thank you for bringing the comments across from #98 too, this is great!
One query: do you think it'd be possible to roll the callback into ComrakOptions? The lifetimes on that thing make me think it might be tricky, but it could be cleaner.
I'm going to merge this right away and leave that in your hands. Thank you so much!
| // Need to borrow the callback from the parser only for the lifetime of the Subject, 'subj, and | ||
| // then give it back when the Subject goes out of scope. Needs to be a mutable reference so we | ||
| // can call the FnMut and let it mutate its captured variables. | ||
| callback: Option<&'subj mut &'c mut dyn FnMut(&[u8]) -> Option<(Vec<u8>, Vec<u8>)>>, |
There was a problem hiding this comment.
Heh. I totally agree that this super ugly! I spent a long time staring at it trying to figure out if it could be made nicer. Unfortunately, it can't because of two things:
- we need a reference to the closure because you can only have one instance of a mutable reference at any given time. You can futher borrow that reference (as we have done here), and the only other alternative is moving the reference around everywhere which is far more ugly!
- you can only use a mutable reference through another mutable reference. You can't use it through an immutable reference. So
&'a &'b mutwould not work.
It's ugly, but it gives us memory safety so I guess it's okay! 😭 😆
|
Thank you! ❤️ I'm really excited to see these changes get released! 🎉 I thought about adding this to the options struct, but I feel like that would only pass on the ugliness to the user (if they store the options) instead of making it any easier to us. This way, the user doesn't need to worry about any of the lifetime madness, we take care of it all internally and they just have to pass a simple callback to our function. :) |
|
Your reasoning is super solid -- thank you! I'll make the point release now 👍 |
|
Done! https://crates.io/crates/comrak/0.4.2 Thank you so much for helping out. 🙏 |
|
Thank you for being so responsive and helping me when I needed it! 😄 🎉 |
|
Amazing work you've been doing @sunjay ! Kudos. |
|
Thank you @brson! Happy to help out! 😄 |
3 participants
Wow! This implementation is so much simpler than #98. I totally get why
pulldown-cmarkwent with this.This PR adds a new method
parse_document_with_broken_link_callbackwhich allows you to pass in a function/closure which will be used to resolve broken reference links. This is similar in spirit (but quite different in reality) than the method thatpulldown-cmarkhas for this.I think the approach I've taken here is even more flexible than
pulldown-cmark. Our closure isFnMutinstead ofFnwhich means that the user can do whatever they want in the closure to resolve the broken reference. We also use byte slices instead of string slices since that is conventionally what is used throughout the rest of this library.The change has been tested locally and I added a doctest that both documents how to use this and to show that it really works.
I copied the additional comments I added in my previous PR to this PR so the codebase still improves even though we went with a different approach. :)
If these changes work and they get merged, feel free to close my other PR. 😄 🎉
Documentation Screenshot
Compatibility
Unlike my previous PR (#98), this PR is actually completely backwards compatible. By adding a new function and implementing the old one in terms of that, none of my changes can break anyone's code. Feel free to just bump the patch version and release this when it is ready. 🎉