mirumee / django-prices

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

Render price without decimal digits if they are equal to 0 #40

Closed szewczykmira closed 7 years ago

szewczykmira commented 7 years ago

Allow templatetags to render price without decimal digits (if they are equal to 0). This change was suggested in issue #32

Example usage: {% gross price normalize=True %}

codecov-io commented 7 years ago

Codecov Report

Merging #40 into master will increase coverage by 1.48%. The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master      #40      +/-   ##
==========================================
+ Coverage   85.63%   87.11%   +1.48%     
==========================================
  Files           5        5              
  Lines         174      194      +20     
  Branches       21       23       +2     
==========================================
+ Hits          149      169      +20     
  Misses         15       15              
  Partials       10       10
Impacted Files Coverage Δ
django_prices/templatetags/prices_i18n.py 93.33% <100%> (+1.84%) :white_check_mark:
django_prices/templatetags/prices.py 83.33% <70%> (+10.6%) :white_check_mark:

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 74dd138...7b929f6. Read the comment docs.

krzysztofwolski commented 7 years ago

Have you tested it with currencies with 0 or 3 decimal places? https://en.wikipedia.org/wiki/ISO_4217

patrys commented 7 years ago

@krzysztofwolski What would you expect to happen if the currency already has zero decimal places?

krzysztofwolski commented 7 years ago

@patrys Well, it would look this same. Just wondered if it was tested after seeing tests using only USD as currency.

szewczykmira commented 7 years ago

@krzysztofwolski it was tested with currencies with 0 decimal digits and I'll push tests with them as soon as I take care of currencies with 3 digits :)

szewczykmira commented 7 years ago

@krzysztofwolski @patrys So for last few hours I was looking for solution how to handle normalization when currency has 3 decimal digits - and I must say - there is no pretty solution for that. Currency formats available through babel don't define 3 decimal digits. We can achieve that only by setting currency_digits=True in format_currency, but it will be always using currency's number of decimal digits (even if normalization is set to true) Right now I have two options: First - don't allow for price normalization when price has 3 decimal digits. Second - make a little hack:

currency_fractions = get_currency_fraction(currency)
if currency_fractions == 3:
    pattern = pattern.replace('.00', '.###')
else:
    pattern = pattern.replace('.00', '.##')

What do you think?

patrys commented 7 years ago

Can't we do something like patter.replace('.00', '.' + '#' * currency_fractions)? Does the pattern always contain .00 even when there are three digits?

szewczykmira commented 7 years ago

I didn't find any pattern that contains three digits. I don't want to mess anything so I made issue in babel repository and I'm waiting for their response.

patrys commented 7 years ago

Babel does not provide its own data, locale data comes from official Unicode releases.

szewczykmira commented 7 years ago

@patrys Note form Unicode about currency pattern: Note: the number of decimals will be set by programs that use CLDR to whatever is appropriate for the currency, so do not change them; keep exactly 2 decimals. Are we sure we want to do against this note?

patrys commented 7 years ago

In this case we are the program. We already know the appropriate number of decimals so we can choose do the following:

  1. When asked to replace two decimal zeroes with two decimal hashes and call it a day.

  2. Always replace two decimal zeroes with either an appropriate number of decimal zeroes or an appropriate number of decimal hashes depending on the currency and whether we were asked for normalization (this is what currency_digits does).

I'm leaning towards the latter as it seems to be the Right Thing To Do™.

szewczykmira commented 7 years ago

Updated with second option.

szewczykmira commented 7 years ago

@patrys can you look at it?