mirumee / django-prices

Django fields for the prices module
158 stars 53 forks source link

Switch to prices version 1.0.1-beta #48

Closed maarcingebala closed 6 years ago

codecov-io commented 6 years ago

Codecov Report

Merging #48 into master will increase coverage by 0.73%. The diff coverage is 95.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #48      +/-   ##
==========================================
+ Coverage      94%   94.73%   +0.73%     
==========================================
  Files           3        3              
  Lines         100       76      -24     
  Branches        9        5       -4     
==========================================
- Hits           94       72      -22     
+ Misses          4        2       -2     
  Partials        2        2
Impacted Files Coverage Δ
django_prices/widgets.py 88.88% <0%> (ø) :arrow_up:
django_prices/templatetags/prices_i18n.py 95.91% <100%> (+2.36%) :arrow_up:
django_prices/templatetags/prices.py 100% <100%> (ø) :arrow_up:

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 41b7721...697f2da. Read the comment docs.

patrys commented 6 years ago

The original idea was to have two field types:

price_net = AmountField(currency='USD')
price_gross = AmountField(currency='USD')
price = PriceField(net='price_net', gross='price_gross')
maarcingebala commented 6 years ago

@patrys Now I'm wondering what's the purpose of the price field in the above example. Having two fields for net and gross, one could simply have a price property that would construct a Price based on the two amounts. Also, how would you recommend to use those fields to someone who doesn't need to take taxes into account?

patrys commented 6 years ago

@elwoodxblues I guess you could also have a property perform what Django's GenericForeignKey does but it's a lot of copy-paste work: you have to implement at least a getter and a setter.

Edit: if you don't need taxes then you should be fine using only Amounts and AmountFields. There's no reason for you to deal with Prices.

maarcingebala commented 6 years ago

I'm only concerned about how intuitive would such API be. If usage of the PriceField is only reasonable with taxes, is the PriceField a proper name then?

patrys commented 6 years ago

Well, Price is named Price so a field that returns a Price is called... a PriceField. We can of course rename Price to something entirely different.

maarcingebala commented 6 years ago

It just feels strange that the price will sometimes be represented as Price and sometimes as Amount. This should probably be discussed in a wider group with other contributors.

patrys commented 6 years ago

@elwoodxblues The only useful feature of Price is to represent separate net and gross amounts though.

maarcingebala commented 6 years ago

@koradon In this PR we should also bump the version of this package to 1.0.0-beta.

koradon commented 6 years ago

@elwoodxblues @patrys pls review