Conversation
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
CodSpeed Performance ReportMerging #115 will not alter performanceComparing Summary
|
samuelcolvin
force-pushed
the
custom-exception
branch
from
5e1fed4 to
d5c83a2
Compare
davidhewitt
left a comment
davidhewitt
left a comment
There was a problem hiding this comment.
Exception type itself looks good, however I'd like to discuss options about how the path is actually built up...
| fn set_key_path(&self, err: PythonJsonError) -> PythonJsonError; | ||
| } | ||
|
|
||
| pub(crate) trait MaybeBuildPath { |
There was a problem hiding this comment.
I understand why you went this way (and map exceptions as they propagate through take_value), though I wonder if it might be better to have a method on Parser to work out the .path() given some index, similar to how we have a method to work out file position. We could probably re-use a lot of the .skip() implementation to achieve it.
I think that'd be the only way to get json-path-to-errors for iterable parsing without keeping wasted state for the 99.99999% of successful parses.
There was a problem hiding this comment.
ye, you're right, we should probably try that, partly because it avoids the parse struct becoming more generic.
| #[derive(Debug)] | ||
| pub struct PythonJsonError { | ||
| pub error: JsonError, | ||
| path: Option<Vec<PathItem>>, |
There was a problem hiding this comment.
Is it worth noting here that the path elements are stored in reverse order? Should we prefer VecDeque or LinkedList so that we can insert into the front and avoid potential ordering mistakes?
| let v = self._check_take_value(py, peek_first)?; | ||
| mut build_path: impl MaybeBuildArrayPath, | ||
| ) -> PythonJsonResult<()> { | ||
| let v = self |
There was a problem hiding this comment.
To avoid having to map all over the place, I wonder whether something like
BuildPath::enter_array(|b: &mut BuildPath::ArrayIndex| { /* do stuff */ })would be more ergonomic, where you call b.set_index() to update the index, and then enter_array can be responsible for adding the path to errors inside the closure.
Similar thought for objects.
... but I still wonder if having some parser.path() method to rebuild the whole path and keep it out of the rest of the control flow is better at the cost of 2x slowdown on errors.
2 participants
Fix #99