Skip to content
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

Improve default= handling on MoneyField #60

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rafalp
Copy link
Contributor

@rafalp rafalp commented Mar 13, 2018

This PR improves our handling for scenario when MoneyField.default is of type Money:

MoneyField.__init__ validates that Money.currency is same as one passed in the currency argument, and raises ValueError otherwise, so its impossible to create model with field as such:

class InvalidModel(models.Model):
    price = MoneyField(
        currency='USD', default=Money('0', 'EUR'), max_digits=9,
        decimal_places=2)

MoneyField.deconstruct tests if default value is instance of Money, and if that is the case, deconstructs kwargs['default'] to str(Money.amount), so migrations generated by Django don't import prices and don't construct explicit Money, 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.

@codecov
Copy link

codecov bot commented Mar 13, 2018

Codecov Report

Merging #60 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           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.

@patrys
Copy link
Contributor

patrys commented Mar 13, 2018

Does that mean that we accept integers/strings as default values? Won't that cause even more problems?

@rafalp
Copy link
Contributor Author

rafalp commented Mar 13, 2018

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.

@patrys
Copy link
Contributor

patrys commented Mar 13, 2018

What would call to_python though? Unless I'm mistaken the constructor does not do that.

@rafalp
Copy link
Contributor Author

rafalp commented Mar 13, 2018

@patrys the path here is roughly Model.__init__() -> MoneyField.get_default() -> MoneyField.to_python() -> DecimalField.to_python.

@maarcingebala maarcingebala removed their request for review September 3, 2019 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants