Add locale to Qif.parse() method #87
Conversation
quiffen/core/transaction.py
Outdated
| @@ -395,7 +396,7 @@ def from_list( | |||
| field_info.split(" ")[0] | |||
| ) | |||
| elif line_code in {"T", "U"}: | |||
| amount = utils.parse_decimal(field_info) | |||
| amount = utils.parse_decimal(field_info, locale=locale) | |||
There was a problem hiding this comment.
The utils.parse_decimal function doesn't take a locale argument, so this change won't do anything
| @@ -109,6 +109,7 @@ def parse( | |||
| separator: str = "\n", | |||
| day_first: bool = False, | |||
| encoding: str = "utf-8", | |||
| locale: str = 'en_US' | |||
There was a problem hiding this comment.
Given the limited number of locales available, would it not be better to use an enum? Perhaps we could have something like:
class DecimalSeparator(enum.Enum):
PERIOD = "."
COMMA = ","
There was a problem hiding this comment.
Babel is a good shout, but please try to be more thorough and consider the wider impacts of the work you're doing. Typically, it's best to discuss changes before implementing them - it's going to be down to me to maintain anything you do that gets merged, and ultimately I need to weigh up the cost of adding new libraries and code.
Usually you'd open an issue first to discuss potential solutions to a problem you're having :)
|
|
||
|
|
||
| Build the Package | ||
| ================= | ||
| Run the following command in the terminal to build the package: | ||
| ```bash | ||
| python setup sdist | ||
| ``` | ||
| This command will create a dist/ directory with a source distribution of your library. | ||
| Install the local package using pip: | ||
| ```bash | ||
| pip install dist/quiffen-1.0.tar.gz | ||
| ``` |
There was a problem hiding this comment.
This is incorrect. Quiffen uses Poetry, so the correct build command is poetry build.
There was a problem hiding this comment.
Also, I'm not sure this is in the scope of this PR. If you'd like to make these changes, please open another PR.
| from setuptools import setup, find_packages | ||
|
|
||
| setup( | ||
| name='quiffen', | ||
| version='1.0', | ||
| packages=find_packages(), | ||
| install_requires=[ | ||
| # Your library dependencies | ||
| "python >= 3.8", | ||
| "pydantic >= 1.10.2", | ||
| "pandas >= 1.5.1", | ||
| "python-dateutil >= 2.8.2", | ||
| "babel >= 2.13.1" | ||
| ], | ||
| ) |
There was a problem hiding this comment.
Not needed. See previous comments.
| @@ -17,6 +17,7 @@ | |||
| from quiffen.core.class_type import Class | |||
| from quiffen.core.investment import Investment | |||
| from quiffen.core.split import Split | |||
| from babel.numbers import parse_decimal, parse_number | |||
There was a problem hiding this comment.
It doesn't look like you're using parse_number here at all.
| amount = utils.parse_decimal(field_info, locale=locale) | ||
| amount = parse_decimal(field_info, locale) |
There was a problem hiding this comment.
The utils.parse_decimal function is used in many other places. It might be better to just change the function in utils, and make sure the locale is passed to all callsites.
Also, it might be worth updating docs to show that locale is an option.
|
Closing due to inactivity |
I had to import qif file with Italian locale (using "," as decimal separator), therefore I have added a parameter to Qif.parse() method to specify the locale.