Handle errors returned by cmark_resume_with_options()
#244
Conversation
Need a small code adjustment to please Byron/pulldown-cmark-to-cmark@13785fe.
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.
| newlines_before_start: 0, | ||
| padding: Vec::new(), | ||
| ..state | ||
| let simplified_state = state.map(|mut state| { |
There was a problem hiding this comment.
This does indeed look cleaner. I did not think that I could work on the state as a mutable within map even though the option itself is immutable. This part looks good to me.
i18n-helpers/src/gettext.rs
Outdated
| translated | ||
| // Translation should always succeed. | ||
| let translated_events = | ||
| translate_events(&events, catalog).expect("Fatal: failed to translate events"); |
There was a problem hiding this comment.
Is this Fatal: prefix needed here? It seems to be inconsistent with the rest of expect() failures we have in this codebase.
There was a problem hiding this comment.
Right I can't see it anywhere else, removed it. And panic is already fatal anyway...
i18n-helpers/src/lib.rs
Outdated
| @@ -462,7 +462,7 @@ fn heuristic_codeblock<'a>( | |||
| _ => true, | |||
| }; | |||
|
|
|||
| if is_translate { | |||
| Ok(if is_translate { | |||
There was a problem hiding this comment.
Nit: wrapping the expression in Ok looks to me a bit harder to read but maybe this is the way to do it in rust (haven't done much rust yet). Will this be more readable if you store the result into two variables - events and ctx, then at the end of the function do Ok((events, ctx))?
There was a problem hiding this comment.
Sure, done! I've written even less rust, so I have no idea what the proper "rust style" would be (or if it there's a preferred way in the first place). But your suggestion looks more readable.
Thanks for handling this, @cip999! Please rebase this on top of latest changes. Would be good if @mgeisler could take a look as well since he has some context on this issue. |
Thanks for reviewing! Yes that's a good idea, and it can wait, there's no rush to merge this PR right now. 🙂 |
cip999
force-pushed
the
fuzz-panic-fix
branch
from
bd55b08 to
9a72a91
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #244 +/- ##
==========================================
- Coverage 84.87% 84.74% -0.13%
==========================================
Files 15 15
Lines 3366 3377 +11
Branches 3366 3377 +11
==========================================
+ Hits 2857 2862 +5
+ Misses 422 413 -9
- Partials 87 102 +15 ☔ View full report in Codecov by Sentry. |
cip999
force-pushed
the
fuzz-panic-fix
branch
from
9a72a91 to
26f859f
Compare
Co-authored-by: Martin Geisler <[email protected]>
|
Thanks for contribution, @cip999! |
Here's the follow-up to Byron/pulldown-cmark-to-cmark#91!