adamcharnock / django-hordak

Double entry accounting in Django
http://django-hordak.readthedocs.io
MIT License
228 stars 55 forks source link

Unicode spec fixes #87

Closed nitsujri closed 1 year ago

nitsujri commented 1 year ago

Currently these specs fail on OSX (possibly others) due to babel's machine dependent output of spaces as unicode \xa0.

Original discussion points:

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 :tada:

Comparison is base (b9a4b65) 92.42% compared to head (d77c009) 92.45%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #87 +/- ## ========================================== + Coverage 92.42% 92.45% +0.02% ========================================== Files 57 57 Lines 3500 3513 +13 Branches 223 222 -1 ========================================== + Hits 3235 3248 +13 Misses 223 223 Partials 42 42 ``` | [Impacted Files](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/87?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Adam+Charnock) | Coverage Δ | | |---|---|---| | [hordak/templatetags/hordak.py](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/87?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Adam+Charnock#diff-aG9yZGFrL3RlbXBsYXRldGFncy9ob3JkYWsucHk=) | `46.59% <100.00%> (+1.24%)` | :arrow_up: | | [hordak/tests/models/test\_core.py](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/87?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Adam+Charnock#diff-aG9yZGFrL3Rlc3RzL21vZGVscy90ZXN0X2NvcmUucHk=) | `100.00% <100.00%> (ø)` | | | [hordak/tests/views/test\_accounts.py](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/87?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Adam+Charnock#diff-aG9yZGFrL3Rlc3RzL3ZpZXdzL3Rlc3RfYWNjb3VudHMucHk=) | `92.07% <100.00%> (+0.41%)` | :arrow_up: | | [hordak/utilities/currency.py](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/87?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Adam+Charnock#diff-aG9yZGFrL3V0aWxpdGllcy9jdXJyZW5jeS5weQ==) | `83.50% <100.00%> (+0.16%)` | :arrow_up: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

nitsujri commented 1 year ago

@PetrDlouhy I thought about the specs and my recommendations are either:

The goal of these specs is to test the __str__() while the job of testing format_currency is left to babel themselves. Making all specs locale agnostic I don't think is realistic since there's just too many different formats and thusly we should leave that to babel's team.

I believe either option is valid, removing the unicode still allows the decimal digit to come through (and thusly be tested) or mocking format_currency to ignore the problem all together.

Thoughts?

PetrDlouhy commented 1 year ago

After some experimenting and thinking about this I think the best solution would be to introduce some new settings that would allow to define the django-hordak's locale. Something like HORDAK_LOCALE variable that would be passed to babel.numbers.format_currency as locale parameter (if set).

Then we would be able to run the tests simply with

@override_settings(HORDAK_LOCALE=`en_US`)

to unify the output of format_currency.

This would not only fix the tests but also allow users to better control the outputs (not only for __str__, but also for currency templatetag filter).

nitsujri commented 1 year ago

new settings that would allow to define the django-hordak's locale. Something like HORDAK_LOCALE variable

For Django, locale is commonly set on a per-request level, so a project level setting wouldn't be helpful. Our project serves 4 asian based locales.

But, I mocked up a quick/janky example of how it could work for Hordak.

It works locally in OSX and on the server. If I'm on the right track, I can clean it up.

PetrDlouhy commented 1 year ago

Yeah, I think solution is superb.

I am only thinking if this can't cause some compatibility issues, but I think the change is not so big so just a note to CHANGES.txt could be enough.

I would also apply similar solution to the templatetag.

nitsujri commented 1 year ago

@PetrDlouhy done!

Let me know if there's anything else.

PetrDlouhy commented 1 year ago

I think it is very good job. Thank you.

There is another occurrence of format_currency and also format_decimal in hordak/templatetags/hordak.py in currency filter. I would expect that we need to change those to make the behavior coherent, what do you think?

If you do that, please add also few simple tests for the currency filter (direct call of that function would be enough) to ensure the new behavior.

nitsujri commented 1 year ago

@PetrDlouhy This is done.

PetrDlouhy commented 1 year ago

@nitsujri Thans. This is great.