Add custom error with variants to `parser` module #50

CalemRoelofs · 2023-11-05T12:11:16Z

Closes #38

This PR adds the parser::errors submodule using thiserror to handle different variants.
In the parser::parser module, almost all calls to unwrap() have been replaced and functions now return Result<T, JsonPathParserError> instead.
Changing the signature of the public method parse_json_path from Result<JsonPath, pest::error::Error<Rule>> to Result<JsonPath, JsonPathParsingError> is considered a breaking change so I'll leave it you you @besok on how you want to handle bumping versions etc.
There's a bit of extra boilerplate added to the existing functions where Option<T> is now Result<Option<T>, JsonPathParserError> but I've tried to make it as concise as possible.

CalemRoelofs · 2023-11-05T12:12:35Z

src/parser/parser.rs

    let mut expr: Option<FilterExpression> = None;
-    while pairs.peek().is_some() {
-        let next_expr = parse_logic_and(pairs.next().unwrap().into_inner());
+    let error_message = format!("Failed to parse logical expression: {:?}", pairs);


error_message has to be constructed before the call to pair.into_inner() as into_inner() takes ownership and moves the value

CalemRoelofs · 2023-11-05T12:13:54Z

src/parser/parser.rs

+            Rule::logic => parse_logic(pairs.next().unwrap().into_inner()),
+            Rule::atom => {
+                let left: Operand = parse_atom(pairs.next().unwrap())?;
+                if pairs.peek().is_none() {
+                    Ok(FilterExpression::exists(left))
+                } else {
+                    let sign: FilterSign = FilterSign::new(pairs.next().unwrap().as_str());
+                    let right: Operand = parse_atom(pairs.next().unwrap())?;
+                    Ok(FilterExpression::Atom(left, sign, right))
+                }


We're still using .unwrap() here but it should be safe as we're peeking ahead to make sure there's a value.

yeah, no problem. But can you replace unwrap with expect("unreachable in arithmetic") or something like that

besok · 2023-11-05T13:45:59Z

src/parser/parser.rs

    match rule.as_rule() {
-        Rule::path => rule
+        Rule::path => Ok(rule
            .into_inner()
            .next()


in this case, I would suggest

next() .ok_or(...) .and_then(parse_internal)

instead of

next() .map() .unwrap_or .unwrap_or

besok · 2023-11-05T13:46:28Z

src/parser/parser.rs

-            .unwrap_or(JsonPath::Empty),
-        Rule::current => JsonPath::Current(Box::new(
+            .unwrap_or(Ok(JsonPath::Empty))
+            .unwrap_or(JsonPath::Empty)),


Basically, since we have Result as a result we don't have to return JsonPath::Empty and can return Err instead (apparently). I would suggest replacing unwrap_or everywhere to ok_or(Err(..))

besok · 2023-11-05T13:47:32Z

src/parser/parser.rs

-        Rule::current => JsonPath::Current(Box::new(
+            .unwrap_or(Ok(JsonPath::Empty))
+            .unwrap_or(JsonPath::Empty)),
+        Rule::current => Ok(JsonPath::Current(Box::new(
            rule.into_inner()
                .next()
                .map(parse_internal)


here is the same:

we convert the first option into result using ok_or

then we use flatmap (and_then) instead of map

besok · 2023-11-05T13:49:19Z

src/parser/parser.rs

-        _ => JsonPath::Empty,
+            .unwrap_or(JsonPath::Empty)),
+        Rule::index => Ok(JsonPath::Index(parse_index(rule)?)),
+        _ => Ok(JsonPath::Empty),


here we return Error instead of JsPath::Empty I think

besok · 2023-11-05T13:54:52Z

src/parser/parser.rs

+            Rule::logic => parse_logic(pairs.next().unwrap().into_inner()),
+            Rule::atom => {
+                let left: Operand = parse_atom(pairs.next().unwrap())?;
+                if pairs.peek().is_none() {
+                    Ok(FilterExpression::exists(left))
+                } else {
+                    let sign: FilterSign = FilterSign::new(pairs.next().unwrap().as_str());
+                    let right: Operand = parse_atom(pairs.next().unwrap())?;
+                    Ok(FilterExpression::Atom(left, sign, right))
+                }


yeah, no problem. But can you replace unwrap with expect("unreachable in arithmetic") or something like that

besok · 2023-11-05T14:09:25Z

Closes #38

This PR adds the parser::errors submodule using thiserror to handle different variants. In the parser::parser module, almost all calls to unwrap() have been replaced and functions now return Result<T, JsonPathParserError> instead. Changing the signature of the public method parse_json_path from Result<JsonPath, pest::error::Error<Rule>> to Result<JsonPath, JsonPathParsingError> is considered a breaking change so I'll leave it you you @besok on how you want to handle bumping versions etc. There's a bit of extra boilerplate added to the existing functions where Option<T> is now Result<Option<T>, JsonPathParserError> but I've tried to make it as concise as possible.

The PR looks very good. Nice job! I added just a couple of comments, just to correct a little.

Changing the signature of the public method parse_json_path from Result<JsonPath, pest::error::Error<Rule>> to Result<JsonPath, JsonPathParsingError> is considered a breaking change so I'll leave it you you @besok on how you want to handle bumping versions etc.

Yeah, it seems fine, but I would ensure the new error enum has a to_string implementation as we convert it to string here and either change TryInto or add to_string

CalemRoelofs · 2023-11-05T19:10:20Z

@besok thank you for the feedback! I'll implement these changes in the coming week.

CalemRoelofs · 2023-11-10T16:51:24Z

src/parser/parser.rs

-            .unwrap_or(JsonPath::Empty),
-        Rule::index => JsonPath::Index(parse_index(rule)),
-        _ => JsonPath::Empty,
+                .unwrap_or(Ok(JsonPath::Empty))?,


@besok handling the Rule::current variant with ok_or(Err()) causes the following tests to fail:

failures: ---- parser::parser::tests::path_test stdout ---- thread 'parser::parser::tests::path_test' panicked at 'parsing error expected valid Rule::current token but found nothing', src\parser\parser.rs:273:17 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ---- parser::parser::tests::index_filter_test stdout ---- thread 'parser::parser::tests::index_filter_test' panicked at 'parsing error expected valid Rule::current token but found nothing', src\parser\parser.rs:273:17 ---- tests::no_value_chain_test stdout ---- thread 'tests::no_value_chain_test' panicked at 'the path is correct: "expected valid Rule::current token but found nothing"', src\lib.rs:1141:65 ---- tests::no_value_filter_from_not_arr_filter_test stdout ---- thread 'tests::no_value_filter_from_not_arr_filter_test' panicked at 'the path is correct: "expected valid Rule::current token but found nothing"', src\lib.rs:1092:68 failures: parser::parser::tests::index_filter_test parser::parser::tests::path_test tests::no_value_chain_test tests::no_value_filter_from_not_arr_filter_test

I'm not sure if this a problem with the tests or a problem with the logic of their not being a value when calling Rule::current.into_inner(), so I've left it with the previous behavior of unwrap_or(JsonPath::Empty) which was there before.

Me neither :).
No problems. Let's leave it as is. I will take a look at the weekend, hopefully.

CalemRoelofs · 2023-11-10T16:52:50Z

@besok I've refactored out most of the unwrap_or statements, but I'm a bit unsure of what the error messages returned from ok_or actually should be. Open to any and all suggestions 😄

besok · 2023-11-10T17:22:56Z

@CalemRoelofs
Yeah, looks good to me. The messages just say where is the error and we can have a look there. It is enough I think.

besok · 2023-11-10T17:26:46Z

Ah, the build error with Clippy, you need to fix a small warning
Just run locally:

cargo clippy --workspace --tests --all-features -- -D warnings

CalemRoelofs · 2023-11-10T17:33:28Z

Ah, the build error with Clippy, you need to fix a small warning Just run locally:
cargo clippy --workspace --tests --all-features -- -D warnings

Fixed now :)

besok · 2023-11-10T17:38:59Z

thank you very much for the PR!

Add custom error with variants to parser module

5c45d75

CalemRoelofs commented Nov 5, 2023

View reviewed changes

besok reviewed Nov 5, 2023

View reviewed changes

Replace unwrap_or with proper Result handling

1ca985c

CalemRoelofs commented Nov 10, 2023

View reviewed changes

Fix clippy warning

b30d953

besok approved these changes Nov 10, 2023

View reviewed changes

besok merged commit 2cfb303 into besok:main Nov 10, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add custom error with variants to `parser` module #50

Add custom error with variants to `parser` module #50

CalemRoelofs commented Nov 5, 2023

CalemRoelofs Nov 5, 2023

besok Nov 5, 2023

CalemRoelofs Nov 5, 2023 •

edited

Loading

besok Nov 5, 2023

besok Nov 5, 2023

besok Nov 5, 2023

besok Nov 5, 2023

besok Nov 5, 2023

besok Nov 5, 2023

besok commented Nov 5, 2023

CalemRoelofs commented Nov 5, 2023

CalemRoelofs Nov 10, 2023

besok Nov 10, 2023 •

edited

Loading

CalemRoelofs commented Nov 10, 2023

besok commented Nov 10, 2023

besok commented Nov 10, 2023

CalemRoelofs commented Nov 10, 2023

besok commented Nov 10, 2023

Add custom error with variants to parser module #50

Add custom error with variants to parser module #50

Conversation

CalemRoelofs commented Nov 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CalemRoelofs Nov 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

besok commented Nov 5, 2023

CalemRoelofs commented Nov 5, 2023

Choose a reason for hiding this comment

besok Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

CalemRoelofs commented Nov 10, 2023

besok commented Nov 10, 2023

besok commented Nov 10, 2023

CalemRoelofs commented Nov 10, 2023

besok commented Nov 10, 2023

Add custom error with variants to `parser` module #50

Add custom error with variants to `parser` module #50

CalemRoelofs Nov 5, 2023 •

edited

Loading

besok Nov 10, 2023 •

edited

Loading