coddingtonbear / django-measurement

Easily store, retrieve, and convert measurements of weight, volume, distance, area and more.
MIT License
145 stars 33 forks source link

Updated to allow optional use of decimals without breaking current implementations #94

Closed JackAtOmenApps closed 4 years ago

JackAtOmenApps commented 4 years ago

Split MeasurementField into two separate model fields in a way that is transparent to the end user.

This still works as normal, storing float values:

volume = MeasurementField(measurement=Volume)

But now you could also do this to store/retrieve Decimal values:

volume = MeasurementField(measurement=Volume, decimal=True, max_digits=20, decimal_places=16)

I haven't formatted this using black yet, since that was an issue on python-measurement.

Also updated print statements in docs, updated 'super' to the modern format, and added some extra info in the docs about this new feature.

JackAtOmenApps commented 4 years ago

@coddingtonbear and @codingjoe, would you please take a look when you get a chance?

codecov-io commented 4 years ago

Codecov Report

Merging #94 into master will decrease coverage by 29.4%. The diff coverage is 38.02%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #94       +/-   ##
===========================================
- Coverage   89.33%   59.92%   -29.41%     
===========================================
  Files           4        4               
  Lines         150      277      +127     
===========================================
+ Hits          134      166       +32     
- Misses         16      111       +95
Impacted Files Coverage Δ
django_measurement/models.py 51.29% <34.64%> (-43.38%) :arrow_down:
django_measurement/forms.py 75.75% <63.63%> (-3.91%) :arrow_down:
django_measurement/utils.py 90.9% <75%> (-9.1%) :arrow_down:

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 59b7fcd...56af2ab. Read the comment docs.

codingjoe commented 4 years ago

Hm... I am not so sure about this. I'd rather change this to decimal. This requires users to think about it a bit more. We should however check if it's possible to change the type in a migration rather than adding and removing a field, which would result in data loss.

JackAtOmenApps commented 4 years ago

Will work on it this week.

JackAtOmenApps commented 4 years ago

I would love some help with migrations. Will modern versions of django automatically use AlterField if someone runs makemigrations? If so, that should keep the data, but I'm not sure how to appropriately test this.

JackAtOmenApps commented 4 years ago

Now uses this format:

volume = MeasurementField(measurement=Volume, max_digits=20, decimal_places=16)

Tests fail at least in part because this pull assumes merge of pull 32 from python-measurement (https://github.com/coddingtonbear/python-measurement/pull/32)

JackAtOmenApps commented 4 years ago

Closing since @codingjoe made updates in another PR that solve the problem I was aiming to solve.