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

correct copy of attributes #629

Merged

Conversation

tned73
Copy link
Contributor

@tned73 tned73 commented Sep 3, 2021

After an action like adding the attribute decimal_places_display was incorrectly set and it was interfering with formatting in the str. Even the deprecation warning was triggered.
This fix only copy the internal attributes and avoids incorrectly setting them.

Small proof of issue (Version 2.0.1) Also notice formatting problem.

> from djmoney.money import Money
> effe = Money("1.23", "EUR")
> print(effe)

€1.23

> print(effe._decimal_places_display)

None

> effe = effe + effe
> print(effe)

2.46 €

> print(effe._decimal_places_display)

2

@tned73 tned73 force-pushed the feature/correct_copy_attributes branch 2 times, most recently from 48a0f7b to 5b48c6a Compare September 3, 2021 19:01
@tned73 tned73 force-pushed the feature/correct_copy_attributes branch from 5b48c6a to 58885ea Compare September 3, 2021 19:17
Comment on lines 160 to 161
assert one._decimal_places_display is None, "this should be None"
assert one.decimal_places == 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Generally, I'd prefer a bit higher level assertions (at least in new tests) - e.g. how values will be formatted, as these attributes are telling more about implementation details.

assert three.decimal_places == 2


def test_add_copy_display_places():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, it will be nice to use pytest.mark.parametrize as these 3 tests have similar structure

@Stranger6667
Copy link
Collaborator

Thanks for working on this!

@codecov-commenter
Copy link

codecov-commenter commented Sep 3, 2021

Codecov Report

Merging #629 (8f1832f) into master (4999ac8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 8f1832f differs from pull request most recent head c57f3e4. Consider uploading reports for the commit c57f3e4 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #629   +/-   ##
=======================================
  Coverage   97.94%   97.94%           
=======================================
  Files          29       29           
  Lines         971      973    +2     
  Branches      164      165    +1     
=======================================
+ Hits          951      953    +2     
  Misses         13       13           
  Partials        7        7           
Impacted Files Coverage Δ
djmoney/money.py 99.24% <100.00%> (+0.01%) ⬆️

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 4999ac8...c57f3e4. Read the comment docs.

@tned73
Copy link
Contributor Author

tned73 commented Sep 3, 2021

you are welkom, I have changed the tests as best I can. Let me know if it can be better.

@tned73 tned73 force-pushed the feature/correct_copy_attributes branch from c57f3e4 to f6fd044 Compare September 3, 2021 20:12
@Stranger6667 Stranger6667 merged commit 9c2fda9 into django-money:master Sep 4, 2021
@Stranger6667
Copy link
Collaborator

Released in 2.0.2

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

3 participants