besok / jsonpath-rust

Support for json-path in Rust
https://crates.io/crates/jsonpath-rust
MIT License
99 stars 26 forks source link

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

Closed CalemRoelofs closed 11 months ago

CalemRoelofs commented 11 months ago

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.

besok commented 11 months ago

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 commented 11 months ago

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

CalemRoelofs commented 11 months ago

@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 commented 11 months ago

@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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

thank you very much for the PR!