Open rafalp opened 6 years ago
Merging #60 into master will not change coverage. The diff coverage is
n/a
.
@@ Coverage Diff @@
## master #60 +/- ##
=======================================
Coverage 96.15% 96.15%
=======================================
Files 4 4
Lines 104 104
Branches 10 10
=======================================
Hits 100 100
Misses 2 2
Partials 2 2
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update a6e44f6...ce92d56. Read the comment docs.
Does that mean that we accept integers/strings as default values? Won't that cause even more problems?
I don't think it would cause errors. str
would get passed down to DecimalField.to_python
which would happily do Decimal(str)
that we would then pass to Money
as usual.
Still, our API's shouldn't cause raising eyebrows, and seeing how money becomes string in migration may be causing such eyebrow raise. So I've replaced this with just returning Money.amount
, and letting Django take decimal from there.
What would call to_python
though? Unless I'm mistaken the constructor does not do that.
@patrys the path here is roughly Model.__init__()
-> MoneyField.get_default()
-> MoneyField.to_python()
-> DecimalField.to_python
.
This PR improves our handling for scenario when
MoneyField.default
is of typeMoney
:MoneyField.__init__
validates thatMoney.currency
is same as one passed in thecurrency
argument, and raisesValueError
otherwise, so its impossible to create model with field as such:MoneyField.deconstruct
tests if default value is instance ofMoney
, and if that is the case, deconstructskwargs['default']
tostr(Money.amount)
, so migrations generated by Django don't importprices
and don't construct explicitMoney
, which should make them more future-proof.If somebody out there already has model like in our
InvalidModel
change, this will be breaking change for his/her project.