fix with python parsing seen on pydantic-core #54
Conversation
| @@ -117,16 +127,6 @@ impl<'j> PythonParser<'j> { | |||
| } | |||
| Ok(dict.to_object(py)) | |||
| } | |||
| _ => { | |||
There was a problem hiding this comment.
Well, removing this clause did cause errors to fire here about non-exhaustive matching 😉. But I take your point about it not enforcing all datatypes.
I think part of the performance win in #48 was that it effectively removed a large set of branches (from Peak::new) because we have these same set of branches anyway when we decide what to do with the peek's u8. Put another way, we assume that the peek is always valid and defer erroring until after we fail to parse an integer.
It's definitely possible that there's another way to express this which keeps the same performance property, might require a play and a think.
src/python.rs
Outdated
| Peek::Minus | Peek::Infinity | Peek::NaN => { | ||
| let n = self | ||
| .parser | ||
| .consume_number::<NumberAny>(peek.into_inner(), self.allow_inf_nan)?; |
There was a problem hiding this comment.
Maybe one solution is to remove peek.into_inner() and replace it with peek.into_number() which errors when it's not a number? This would probably keep most of the same performance properties, but force you to handle all invalid cases earlier as part of the wildcard / numerical branch.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #54 +/- ##
==========================================
+ Coverage 93.03% 93.06% +0.02%
==========================================
Files 8 8
Lines 1048 1052 +4
==========================================
+ Hits 975 979 +4
Misses 73 73
Continue to review full report in Codecov by Sentry.
|
CodSpeed Performance ReportMerging #54 will not alter performanceComparing Summary
|
| Peek::Null => { | ||
| self.parser.consume_null()?; | ||
| Ok(py.None()) | ||
| } |
There was a problem hiding this comment.
For what it's worth the compiler will probably reorder branches in a match for what it thinks makes most sense anyway (as long as they're not gated by conditionals, only one of these branches will be traversed), so moving this likely does nothing.
This was introduced by #48 (I checked the commit before and tests passed, failed after).
Honestly I'm not sure about this implementation of
Peek, it means no compiler error or warning if you miss cases in a match statement.Maybe we can someone combine storing the raw
u8with an enum @davidhewitt?