Alternative approach for #91 with custom error type by cip999 · Pull Request #93 · Byron/pulldown-cmark-to-cmark

cip999 · 2024-12-14T17:20:34Z

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!

…rigger assertion failures. (Byron#91)" This reverts commit a368f0f.

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.

src/lib.rs

cip999 · 2024-12-14T19:12:41Z

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 · 2024-12-14T19:19:21Z

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.

When `cmark_resume_with_options()`, which serves as basis for the other `cmark*` functions, finds an inconsistent event stream (for example, two consecutive heading start tags), it panics. Introduce a custom error type `Error` in the crate and change the return type of all public functions from `fmt::Result<_>` to `Result<_, Error>`. The next commit adds an integration test.

cip999 · 2024-12-14T19:39:58Z

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 · 2024-12-14T22:13:57Z

src/lib.rs

+    UnexpectedEvent,
+}
+
+impl fmt::Display for Error {


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/.

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 · 2024-12-15T07:47:27Z

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

PR Byron/pulldown-cmark-to-cmark#93 in the cmark_pulldown_to_cmark crate added structure to the error type returned by `cmark_resume_with_options()`. It also made it return an error when the input is an inconsistent stream of `Event`s. Now, instead of panicking, we can propagate the error to the caller and report a more specific error.

Handle errors returned by `cmark_resume_with_options()` PR Byron/pulldown-cmark-to-cmark#93 in the cmark_pulldown_to_cmark crate added structure to the error type returned by `cmark_resume_with_options()`. It also made it return an error when the input is an inconsistent stream of `Event`s. Now, instead of panicking, we can propagate the error to the caller and report a more specific error. Co-authored-by: Martin Geisler <martin@geisler.net>

Revert "fix: harden again manufactured input which could previously t…

a442910

…rigger assertion failures. (Byron#91)" This reverts commit a368f0f.

cip999 force-pushed the resume-error-handling branch 4 times, most recently from 13a70e6 to d0830e4 Compare December 14, 2024 17:33

Byron requested changes Dec 14, 2024

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Show resolved Hide resolved

cip999 force-pushed the resume-error-handling branch 2 times, most recently from ed6d83e to a304185 Compare December 14, 2024 19:04

cip999 added 2 commits December 14, 2024 20:33

Add integration test for CmarkError::UnexpectedEvent

4766cc1

cip999 force-pushed the resume-error-handling branch from a304185 to 4766cc1 Compare December 14, 2024 19:34

cip999 requested a review from Byron December 14, 2024 19:40

mgeisler reviewed Dec 14, 2024

View reviewed changes

Byron merged commit 21f0f5d into Byron:main Dec 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alternative approach for #91 with custom error type#93

Alternative approach for #91 with custom error type#93
Byron merged 3 commits intoByron:mainfrom
cip999:resume-error-handling

cip999 commented Dec 14, 2024

Uh oh!

Byron left a comment

Uh oh!

Uh oh!

Uh oh!

cip999 commented Dec 14, 2024

Uh oh!

Byron commented Dec 14, 2024

Uh oh!

cip999 commented Dec 14, 2024

Uh oh!

mgeisler Dec 14, 2024

Uh oh!

Byron Dec 15, 2024 •

edited

Loading

Uh oh!

Byron commented Dec 15, 2024 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

cip999 commented Dec 14, 2024

Uh oh!

Byron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cip999 commented Dec 14, 2024

Uh oh!

Byron commented Dec 14, 2024

Uh oh!

cip999 commented Dec 14, 2024

Uh oh!

mgeisler Dec 14, 2024

Choose a reason for hiding this comment

Uh oh!

Byron Dec 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Byron commented Dec 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Byron Dec 15, 2024 •

edited

Loading

Byron commented Dec 15, 2024 •

edited

Loading