-
Notifications
You must be signed in to change notification settings - Fork 247
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Implement optional number
to str
coercion
#975
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #975 +/- ##
==========================================
- Coverage 93.14% 93.11% -0.03%
==========================================
Files 106 106
Lines 15763 15791 +28
Branches 25 25
==========================================
+ Hits 14682 14704 +22
- Misses 1075 1081 +6
Partials 6 6
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
CodSpeed Performance ReportMerging #975 will not alter performanceComparing Summary
|
src/input/input_python.rs
Outdated
// final output needs to be Python anyway. | ||
Ok(s) => PyString::new(self.py(), s), | ||
Err(_) => return Err(ValError::new(ErrorTypeDefaults::StringUnicode, self)), | ||
}; | ||
Ok(s.into()) | ||
} else if coerce_numbers_to_str { | ||
if let Ok(py_decimal) = self.lax_decimal(py) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The downside of going through lax_decimal
will be performance - you're creating a whole new Python decimal
on the way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is. We knew it will be less performant feature anyway.
It hits this only if it's not a string which is already the "bad" case and the alternative is hitting several TypeError exceptions. The alternative is to almost copy the lax_decimal
code.
I'll give it another try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented using just isinstance
calls.
0c44176
to
e0c3432
Compare
9ec8778
to
3156a08
Compare
please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me! Just some small nits...
please update |
Co-authored-by: David Hewitt <[email protected]>
Co-authored-by: David Hewitt <[email protected]>
please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
Change Summary
Implement optional
number
tostr
coercionRelated issue number
See pydantic/pydantic#6045
Checklist
pydantic-core
(except for expected changes)Selected Reviewer: @davidhewitt