partial parsing with JsonValue
#157
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #157 +/- ##
==========================================
+ Coverage 90.94% 91.90% +0.96%
==========================================
Files 12 12
Lines 2042 2138 +96
Branches 2042 2138 +96
==========================================
+ Hits 1857 1965 +108
+ Misses 112 109 -3
+ Partials 73 64 -9
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
CodSpeed Performance ReportMerging #157 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
The implementation... seems correct, but I think it could have been simpler.
Yeah, the recursion loop is a bit weird and complex (could probably be refactored better) but it does avoid recursive function calls at the benefit of bounded stack space (i.e. no segfaults from stack overflow) and higher performance 😂
| | JsonErrorType::ExpectedListCommaOrEnd | ||
| | JsonErrorType::ExpectedObjectCommaOrEnd |
There was a problem hiding this comment.
I wonder, how does this influence cases of invalid JSON like
[1, 2, 3,,, 4, 5, // do we get [1, 2, 3] ?
{"a": 1,, "b": 2. // do we get {"a": 1} ?
| peek = next_peek; | ||
| continue; | ||
| } | ||
| Err(e) if !(allow_partial && e.allowed_if_partial()) => return Err(e), |
There was a problem hiding this comment.
Rather than having every possible ? replaced in this way, I feel like there might have been a solution where the whole loop body goes into a sub-function, and then can match once on a possible error and if it's a partial end, wind up the recursion stack to build the partial datastructure.
But I'm a bit tired to think in more detail or push a useful commit to that effect tonight.
@davidhewitt please review this when you have a chance.
The weird recursive thing we do in
value.rsmakes this very ugly, but it seems to be working.