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
add flag to allow only finite float values #228
Conversation
Codecov Report
@@ Coverage Diff @@
## main #228 +/- ##
==========================================
+ Coverage 98.66% 98.69% +0.02%
==========================================
Files 48 48
Lines 5031 5053 +22
Branches 35 35
==========================================
+ Hits 4964 4987 +23
+ Misses 67 66 -1
Continue to review full report at Codecov.
|
src/input/input_json.rs
Outdated
@@ -115,23 +115,29 @@ impl<'a> Input<'a> for JsonInput { | |||
} | |||
} | |||
|
|||
fn strict_float(&self) -> ValResult<f64> { | |||
fn strict_float(&self, _only_finite: bool) -> ValResult<f64> { |
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.
doesn't make sense for strict mode of JSON since a string is required to represent not finite values
tests/validators/test_float.py
Outdated
), | ||
], | ||
) | ||
def test_non_finite_json_values(py_and_json: PyAndJson, input_value, only_finite, expected): |
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 test is a bit verbose but more explicit and readable than with more parametrization. Do not hesitate to ask for a change or do it yourself @samuelcolvin.
@tiangolo are you happy with such a flag? |
4f976fc
to
42781c4
Compare
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.
Otherwise the idea looks good.
Thanks for the review! Feedback taken |
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, just waiting for @tiangolo's feedback.
@@ -59,6 +59,7 @@ class DictSchema(TypedDict, total=False): | |||
|
|||
class FloatSchema(TypedDict, total=False): | |||
type: Required[Literal['float']] | |||
allow_inf_nan: bool # whether 'NaN', '+inf', '-inf' should be forbidden. default: True |
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.
You need to add allow_inf_nan
to Config
to otherwise it can't be set, might be good to add a test to confirm setting allow_inf_nan
via config works.
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.
Ah yes I added the implementation but not the interfaces. I'll do that soon(ish)
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.
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.
I don't know Rust but this is great! 🚀
this is great, thanks so much. |
fix #224