Alternative approach for #91 with custom error type

[cip999]

This implements what I outlined in this comment.

One thought/remark I have is that I would have liked to add some information to the UnexpectedEvent variant, in particular:

pub enum CmarkError<'a> {
  FormatFailed(fmt::Error),
  UnexpectedEvent(Event<'a>),
}

I could make this work, but then I realized that it would most certainly break all consumers of this library, even those that treat the error as a Box<dyn std::error::Error>. This is because, if the Events are local to the function that calls cmark(), that function cannot simply propagate the error to the caller (e.g. via ?), since then it would outlive the events. The "stupicat" example would also fail to build for this exact reason.

So, I decided to not overdo it. Hope it's okay!

[Byron]

Thanks a lot for tackling this, much better!

There are just a couple of small changes I'd have to request - if you don't have the time please let me know and I can wrap this PR up myself.

Could you add missing_docs to the deny list at the top of lib.rs? That should prevent missing docs in future. Feel free to add #[allow(missing_docs)] on CmarkError then, no need to document the variants.

And now that I read this, could you rename CmarkError to Error?

Further, I think the commit adding the error should have fix!: or feat!: as subject prefix so this will be a breaking change automatically when I release.

An that would already be it.

This is because, if the Events are local to the function that calls cmark(), that function cannot simply propagate the error to the caller (e.g. via ?), since then it would outlive the events. The "stupicat" example would also fail to build for this exact reason.

I understand - it would certainly be nice to have a fully-owned version of Event that could be created with to_owned().
In theory, if you really want it, you could add a serde-json feature, which in turn enables the serde feature in pulldown-cmark. Then Event can be serialized into JSON and handed over that way. It's probably overkill, but it would work.

[cip999]

Thanks for the review! I can definitely follow up with the PR. 🙂

Could you add missing_docs to the deny list at the top of lib.rs? That should prevent missing docs in future. Feel free to add #[allow(missing_docs)] on CmarkError then, no need to document the variants.

There are lots of pre-existing public items without documentation though — I agree that it's a good idea to add it but maybe it's better to do it as a separate change?

And now that I read this, could you rename CmarkError to Error?

Further, I think the commit adding the error should have fix!: or feat!: as subject prefix so this will be a breaking change automatically when I release.

Done both.

In theory, if you really want it, you could add a serde-json feature, which in turn enables the serde feature in pulldown-cmark. Then Event can be serialized into JSON and handed over that way. It's probably overkill, but it would work.

I see, I can do it in another PR perhaps. Although the more I think about it, the more it smells of YAGNI. 🙃

[Byron]

Thanks for the review! I can definitely follow up with the PR. 🙂

Maybe a misunderstanding, but there shouldn't be anything to follow up. Please feel free to help me understand.

Could you add missing_docs to the deny list at the top of lib.rs? That should prevent missing docs in future. Feel free to add #[allow(missing_docs)] on CmarkError then, no need to document the variants.

There are lots of pre-existing public items without documentation though — I agree that it's a good idea to add it but maybe it's better to do it as a separate change?

Apologies, I wasn't aware - it's so unlike me. Please scratch that then.

In theory, if you really want it, you could add a serde-json feature, which in turn enables the serde feature in pulldown-cmark. Then Event can be serialized into JSON and handed over that way. It's probably overkill, but it would work.

I see, I can do it in another PR perhaps. Although the more I think about it, the more it smells of YAGNI. 🙃

Good, I agree.

Then it's only the write_str removal missing, and it can be merged.

Thanks again.

[cip999]

Thanks for the review! I can definitely follow up with the PR. 🙂

Maybe a misunderstanding, but there shouldn't be anything to follow up. Please feel free to help me understand.

Sorry, bad choice of words from me, I meant that I can apply the changes you requested (in reply to "if you don't have the time please let me know and I can wrap this PR up myself").

Could you add missing_docs to the deny list at the top of lib.rs? That should prevent missing docs in future. Feel free to add #[allow(missing_docs)] on CmarkError then, no need to document the variants.

There are lots of pre-existing public items without documentation though — I agree that it's a good idea to add it but maybe it's better to do it as a separate change?

Apologies, I wasn't aware - it's so unlike me. Please scratch that then.

No worries at all!

[mgeisler]

Depending on the policy for new dependencies in pulldown-cmark-to-cmark, I think you could consider using thiserror here. It will basically expand to the hand-written code here.

See https://docs.rs/thiserror/latest/thiserror/.

[Byron]

I'd want to avoid adding dependencies here.
In general, I want to use thiserror less, but if there was a library with a lot of error handling, right now there would be no way around it.

[Byron]

Thanks a lot! This is it, let's merge and create a new release.
https://github.com/Byron/pulldown-cmark-to-cmark/releases/tag/v20.0.0