adamcharnock / django-hordak

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

allow set number of decimal_places in settings #21

Closed PetrDlouhy closed 6 years ago

PetrDlouhy commented 6 years ago

I would like to set more decimal places in my project (because we want to transfer very little amounts of money). This adds possibility to set that in settings. Problem of my approach is, that user will need to set that before migrating (or create his own migration after setting that).

@adamcharnock What do you think about this? Do you see any better way? If not, I will write some note about this to docs before you merge it.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.002%) to 95.448% when pulling 786e983b696e97e7071125f7a4ac77efc6b91177 on PetrDlouhy:editable_decimal_places into 29ec4a9e67a75f2383b9b140a417948662976e52 on adamcharnock:master.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.002%) to 95.448% when pulling 18ed6f0843e0014259a489fdcf757e01bc1b2243 on PetrDlouhy:editable_decimal_places into 29ec4a9e67a75f2383b9b140a417948662976e52 on adamcharnock:master.

adamcharnock commented 6 years ago

Hey @PetrDlouhy, thank you for another PR! You raise a very good point here, I think this is something Hordak definitely needs to support.

My thoughts are:

  1. The forms will also need updated, search the source for decimal_places=2
  2. This could also be a good time to consider adding a setting for the maximum currency value size too (i.e. max_digits, docs). This is particularly relevant for currencies such as YEN/WON which have larger values.
  3. I cannot think of another sensible way to implement this, I think your settings-in-migration is probably the best approach.
  4. When decimal_places=3 I believe the currency will always render as 1.000, rather than 1.00. I think it is ok for Hordak to do this, as Hordak aims to provide core functionality rather than have opinions on rendering. I wanted to flag it here regardless though.

I therefore suggest:

Let me know if you don't have the time or inclination to do any of these and I'll be happy to take a look!

PetrDlouhy commented 6 years ago

@adamcharnock Everything done. Please review.

adamcharnock commented 6 years ago

Amazing, that was super-fast, thank you very much! 👍I'm going to merge now. I'll make a couple of tweaks myself then do a release.

adamcharnock commented 6 years ago

Hey @PetrDlouhy,

Just to update you on what I've done:

Looking at the code I realised that I had been ignoring the hordak.defaults module for a while, so I've moved the settings (and their associated defaults) into there. I've also updated the wording in the docs re the new settings. I hope that is ok with you.

I've also applied the black code formatter, so there have been some extensive but superficial changes there too. Probably worth pulling soon.

If it looks ok to you I'll do a release shortly.

Again, thank you very much for the pull request!

PetrDlouhy commented 6 years ago

@adamcharnock Everything is OK with me. Only, maybe it would be nice to, mention the MIGRATION_MODULES setting, which could be helpful in case of generating own migration.

adamcharnock commented 6 years ago

Done! Version 1.8.1 released 👍