Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement try_run and error propagation #19

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

implement try_run and error propagation #19

wants to merge 3 commits into from

Conversation

rlidwka
Copy link
Collaborator

@rlidwka rlidwka commented Jun 7, 2023

see discussion in #11

This PR adds a new function MarkdownIt::try_parse(), which propagates errors from user plugins.

As an example of one such error, syntect plugin now reports non-existent language tag in try_parse() (but still returns correct output when running parse()):

let mut md = MarkdownIt::new();
markdown_it::plugins::cmark::add(&mut md);
markdown_it::plugins::extra::syntect::add(&mut md);

let input = r#"
``` non-existent-language
hello
```
"#;

let html = md.parse(input).render();
dbg!(html);
// html = "<pre style=\"background-color:#ffffff;\">\n<span style=\"color:#323232;\">hello\n</span></pre>\n"

let html = md.try_parse(input).map(|ast| ast.render());
dbg!(html);
// html = Err(
//    Error {
//        context: "core rule `SyntectRule` returned an error",
//        source: "syntax not found for language `non-existent-language`",
//    },
//)

This commit adds a Result<> to all walk callbacks.

In order to use this, you should change all `Node::walk_*` methods
in your code the following way:

```rust
// replace this:
node.walk(|node, _| {
    dbg!(node);
});

// with this (unwrap is safe here because walk only
// returns error when your function does):
node.walk(|node, _| {
    dbg!(node);
    Ok(())
}).unwrap();
```
@codecov-commenter
Copy link

codecov-commenter commented Jun 7, 2023

Codecov Report

Patch coverage: 90.18% and project coverage change: -0.06 ⚠️

Comparison is base (e4b6beb) 94.12% compared to head (c1bdfba) 94.07%.

❗ Current head c1bdfba differs from pull request most recent head 32b33e8. Consider uploading reports for the commit 32b33e8 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #19      +/-   ##
==========================================
- Coverage   94.12%   94.07%   -0.06%     
==========================================
  Files          57       57              
  Lines        2453     2685     +232     
==========================================
+ Hits         2309     2526     +217     
- Misses        144      159      +15     
Impacted Files Coverage Δ
src/plugins/extra/heading_anchors.rs 0.00% <0.00%> (ø)
src/plugins/extra/linkify.rs 95.38% <83.33%> (-1.34%) ⬇️
src/parser/block/mod.rs 85.88% <84.78%> (-2.76%) ⬇️
src/parser/block/builtin/block_parser.rs 91.30% <86.66%> (-8.70%) ⬇️
src/parser/main.rs 87.93% <86.84%> (-4.07%) ⬇️
src/parser/inline/mod.rs 84.94% <87.75%> (-1.33%) ⬇️
src/plugins/sourcepos.rs 93.75% <88.88%> (-6.25%) ⬇️
src/parser/inline/builtin/inline_parser.rs 95.23% <90.90%> (-4.77%) ⬇️
src/common/ruler.rs 85.04% <100.00%> (+6.29%) ⬆️
src/common/typekey.rs 100.00% <100.00%> (ø)
... and 9 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@begleynk
Copy link

begleynk commented Jun 11, 2023

This looks really cool!

Wanted to clarify something from my comment in #11: At least in my case, even if there are some errors in my plugins, it may not stop us from rendering something. The node itself could render its error into the HTML, but it's still nice to be able to know in the return type that an error occurred. Also, there could be multiple errors in the document, and it would be nice to get them all in one pass. This unfortunately doesn't really fit into the Rust Result model where you return early at the first error encountered.

This may be a bit of a specialized use case, so I understand if it's something you'd not want to add to the library - there's ways around it using the old parse function, but thought I'd flag it in case you found it useful.

Thanks for all your work on this library!

@rlidwka
Copy link
Collaborator Author

rlidwka commented Jun 11, 2023

Also, there could be multiple errors in the document, and it would be nice to get them all in one pass.

That use-case is already supported with parse() function, see code below. You can accumulate errors as metadata in AST, then collect them all afterwards.

Note that there's a bug currently with inline rules, which I just fixed in master branch.

use markdown_it::parser::block::{BlockRule, BlockState};
use markdown_it::parser::core::CoreRule;
use markdown_it::parser::extset::NodeExt;
use markdown_it::parser::inline::{InlineRule, InlineState};
use markdown_it::{MarkdownIt, Node};

#[derive(Debug, Default)]
struct NodeErrors(Vec<anyhow::Error>);
impl NodeExt for NodeErrors {}

struct MyInlineRule;
impl InlineRule for MyInlineRule {
    const MARKER: char = '@';

    fn run(state: &mut InlineState) -> Option<(Node, usize)> {
        let err = state.node.ext.get_or_insert_default::<NodeErrors>();
        err.0.push(anyhow::anyhow!("inline error"));
        None
    }
}

struct MyBlockRule;
impl BlockRule for MyBlockRule {
    fn run(state: &mut BlockState) -> Option<(Node, usize)> {
        let err = state.node.ext.get_or_insert_default::<NodeErrors>();
        err.0.push(anyhow::anyhow!("block error"));
        None
    }
}

struct MyCoreRule;
impl CoreRule for MyCoreRule {
    fn run(root: &mut Node, _md: &MarkdownIt) {
        let err = root.ext.get_or_insert_default::<NodeErrors>();
        err.0.push(anyhow::anyhow!("core error"));
    }
}

fn main() {
    let md = &mut markdown_it::MarkdownIt::new();
    markdown_it::plugins::cmark::add(md);

    md.inline.add_rule::<MyInlineRule>();
    md.block.add_rule::<MyBlockRule>();
    md.add_rule::<MyCoreRule>();

    let text1 = r#"*hello @world*"#;
    let ast = md.parse(text1);

    println!("parsed as:");
    println!("{}", ast.render());

    println!("errors:");
    ast.walk(|node, _| {
        if let Some(errors) = node.ext.get::<NodeErrors>() {
            for error in errors.0.iter() {
                println!(" - {error:?}");
            }
        }
    });
}

Of course the problem with that approach is that errors are not standardized, i.e. two plugins from different authors would report errors in different ways.

@rlidwka
Copy link
Collaborator Author

rlidwka commented Jun 11, 2023

This PR addresses specifically the need to terminate parsing early.

I don't know if there is such a need, and if this is the best approach. Maybe we can use a single parse() function and report errors via function like state.report_error() (which can either stop parsing or collect multiple errors, depending on the settings).

@begleynk
Copy link

That use-case is already supported with parse() function, see code below. You can accumulate errors as metadata in AST, then collect them all afterwards.

Makes perfect sense 👍 I hadn't thought about using a NodeExt - thanks for the example. This approach works perfectly for my use case.

I don't know if there is such a need, and if this is the best approach.

Yeah I'm not sure about the best option myself either. Maybe someone has that specific use case and they could comment here? Apologies if my comment ended up causing extra work! Should have been more explicit originally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants