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

Better default setup on MoneyField for models #661

Merged
merged 3 commits into from Jun 5, 2022

Conversation

flaeppe
Copy link
Contributor

@flaeppe flaeppe commented Mar 8, 2022

Hi, I was digging in to making some progress on #638 (refs: #621) and realised that there was some adjustments that could be made for defaults on djmoney.models.fields.MoneyField. I've gathered several (minor) fixes in this PR(see list below), all of them originating from the same method.

  • Removes possibility of allowing any currency code on string based default value
  • Having a default as type bytes is now properly supported
  • Asserts on incompatible defaults with default currency being None when amount has a valid type (i.e. a default setup generating Money(10, None))

I think the changes here pushes work for #638 at least a little bit forward, as one of the major changes there would (probably?) be setting DEFAULT_CURRENCY=None per default. While the changes here help out on management of MoneyField(..., default_currency=None). And I was thinking it could be nice taking the opportunity helping reduce the size of the work on #638.

Anyways, let me know what you think.

@codecov-commenter
Copy link

codecov-commenter commented Mar 8, 2022

Codecov Report

Merging #661 (cdb96a1) into main (90ea4ad) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #661   +/-   ##
=======================================
  Coverage   97.79%   97.80%           
=======================================
  Files          29       29           
  Lines         997     1001    +4     
  Branches      196      197    +1     
=======================================
+ Hits          975      979    +4     
  Misses         15       15           
  Partials        7        7           
Impacted Files Coverage Δ
djmoney/models/fields.py 100.00% <100.00%> (ø)

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 90ea4ad...cdb96a1. Read the comment docs.

@flaeppe
Copy link
Contributor Author

flaeppe commented Mar 8, 2022

I'm not sure why CI runs with pypy3 now has an sqlite version that's incompatible with django.

@flaeppe
Copy link
Contributor Author

flaeppe commented Mar 8, 2022

I'm not sure why CI runs with pypy3 now has an sqlite version that's incompatible with django.

I think the closest I got to is somewhere here: https://stackoverflow.com/questions/65476852/make-pypy-in-virtualenv-use-shared-library-from-os-rather-than-its-own-copy-lib

No idea if deleting PyPy's copies of libsqlite3.so would actually help though..

@flaeppe
Copy link
Contributor Author

flaeppe commented Mar 21, 2022

@Stranger6667 any insight on why running with pypy would fail on the sqlite version? Do you know if we could do something to resolve it?

Additionally, there's an odd error (new to me at least) on the pre-commit job. Can be seen here: https://github.com/django-money/django-money/runs/5633124684?check_suite_focus=true . Not sure if rerunning the workflow would avoid it

@flaeppe
Copy link
Contributor Author

flaeppe commented Apr 10, 2022

@Stranger6667 any insight on why running with pypy would fail on the sqlite version? Do you know if we could do something to resolve it?

Additionally, there's an odd error (new to me at least) on the pre-commit job. Can be seen here: https://github.com/django-money/django-money/runs/5633124684?check_suite_focus=true . Not sure if rerunning the workflow would avoid it

Seems that it was fixed with actions/setup-python action bumping pypy version for Ubuntu20 (actions/runner-images@1ede808)

Copy link
Contributor

@benjaoming benjaoming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid work 💯

If @Stranger6667 isn't around for another review, I'll get it in within the next days - meanwhile, release notes for this are more than welcome :)

Copy link
Collaborator

@Stranger6667 Stranger6667 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pinging me @benjaoming :)

I have a small comment on one of the error messages

tests/test_models.py Outdated Show resolved Hide resolved
- Removes possibility of allowing _any_ currency code on string based
  default value
- Having a default as type `bytes` is now properly supported
- Asserts on incompatible defaults with default currency being `None`
  when amount has a valid type (i.e. a default setup generating
  `Money(10, None)`)
- Turns off coverage reporting when test run failed
@benjaoming benjaoming merged commit a6867ea into django-money:main Jun 5, 2022
@benjaoming
Copy link
Contributor

Thanks for the patience @flaeppe 🙏 ⏳ 🚀

@flaeppe flaeppe deleted the fix/default-setup branch June 5, 2022 11:01
@flaeppe flaeppe mentioned this pull request Jun 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants