fix #2293: Properly encode Decimals without any decimal places. #2294
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2294 +/- ##
==========================================
- Coverage 99.97% 99.90% -0.08%
==========================================
Files 23 25 +2
Lines 4496 5043 +547
Branches 909 1032 +123
==========================================
+ Hits 4495 5038 +543
Misses 1 1
- Partials 0 4 +4
Continue to review full report at Codecov.
|
|
@PrettyWood @samuelcolvin Any thoughts on this? |
changes/2293-hultner.md
Outdated
| @@ -0,0 +1 @@ | |||
| fix #2293: Properly encode `Decimal` with, or without any decimal places. | |||
There was a problem hiding this comment.
| fix #2293: Properly encode `Decimal` with, or without any decimal places. | |
| Properly encode `Decimal` with, or without any decimal places. |
tests/test_decimal.py
Outdated
| @@ -0,0 +1,28 @@ | |||
| """ | |||
There was a problem hiding this comment.
I know the test files could do with cleaning up, but this doesn't require a standalone test file.
Please move into test_json.
There was a problem hiding this comment.
Have done that now :)
samuelcolvin
added
the
awaiting author revision
|
@samuelcolvin Thanks for the feedback! I know that you are busy. 😃 Does it look good now? |
tests/test_json.py
Outdated
| decimal_places = 0 | ||
| ge = 0 | ||
|
|
||
| ObjId = Id |
There was a problem hiding this comment.
This is unnecssary, better to use condecimal anyway.
There was a problem hiding this comment.
You mean the ConstrainedDecimal subclass? I'm cursion to hear why is condecimal better?
I use the subclasses since typers doesn't like the id: condecimal(args) approach, and it's neat with reusable types.
tests/test_json.py
Outdated
|
|
||
| class Obj(BaseModel): | ||
| id: ObjId | ||
| name: str |
There was a problem hiding this comment.
this field is not relevant to the test.
There was a problem hiding this comment.
Yes, I'll remove it.
tests/test_json.py
Outdated
| test_obj = Obj(id=1, name='Test Obj') | ||
| cycled_obj = Obj.parse_raw(test_obj.json()) | ||
| assert test_obj == cycled_obj |
There was a problem hiding this comment.
This is smart, but not the cleanest way of writing tests. Tests should demonstrate what they're checking explicitly.
Better to do something like
assert Obj(id=1).json() == '{"id": 1, "price": 0.01}'There was a problem hiding this comment.
I wanted to include this since the problematic behaviour were strictly related to encoding and then parsing an object.
But I'll change it if you prefer the other way. However it doesn't capture that parsing the object back also works so maybe add a second parse as well?
Using your method that could look like this.
assert Obj(id=1).json() == '{"id": 1, "price": 0.01}'
assert Obj.parse_raw('{"id": 1, "price": 0.01}') == Obj(id=1)Edit: Small fix in raw json.
|
@samuelcolvin I updated the test and responded to your comments :) |
|
This is great, thank you. |
fix pydantic#2293: Properly encode Decimals without any decimal places. (pydantic#2294)
Change Summary
Adds a custom encoder for Decimals which handles both decimals with decimal places as well as those without.
Related issue number
Closes: #2293
Checklist
Tests where at 99% when i ran them before my change, I do have tests covering my changes
changes/<pull request or issue id>-<github username>.mdfile added describing change(see changes/README.md for details)