Fix sqlite decimal warning #129
Conversation
sea-kelp
requested review from
AetherUnbound,
mpuckett159,
sarayourfriend and
vertexactual
| def parse_decimal(value: Optional[Union[str, Decimal]]) -> Optional[Decimal]: | ||
| if value == Decimal(0) or value: | ||
| return Decimal(value) | ||
| return None |
There was a problem hiding this comment.
Is the goal of this function just to filter None? Other than None, all the potential inputs I can think of seem to be equivalent to just using the Decimal constructor:
| input | parse_decimal(input) | Decimal(input) |
|---|---|---|
| 0 | 0 | 0 |
| 1 | 1 | 1 |
| 1.1 | 1.1 | 1.1 |
| 0.0 | 0.0 | 0.0 |
| 0.01 | 0.01000000000000000020816681711721685132943093776702880859375 | 0.01000000000000000020816681711721685132943093776702880859375 |
| 0.0 | 0 | 0 |
| 0.0 | 0 | 0 |
| -1 | -1 | -1 |
| 0 | 0 | 0 |
| 1000.01 | 1000.009999999999990905052982270717620849609375 | 1000.009999999999990905052982270717620849609375 |
It seems like the function might be able to be simplified (from a reader's perspective) like this:
| def parse_decimal(value: Optional[Union[str, Decimal]]) -> Optional[Decimal]: | |
| if value == Decimal(0) or value: | |
| return Decimal(value) | |
| return None | |
| def parse_decimal(value: Optional[Union[str, Decimal]]) -> Optional[Decimal]: | |
| return Decimal(value) if value is not None else None |
What do you think?
There was a problem hiding this comment.
I'm not familiar with this part of the code at all but the type hints seem to suggest that this function takes both string and decimal, so I think the original function might have been trying to filter out empty string as well as None. Does that make sense?
There was a problem hiding this comment.
Ahh, you're right, I totally missed that in my test. Decimal("") throws an error 👍
There was a problem hiding this comment.
Great work! I've tested that this works using the following:
just fresh-start- Navigate to http://localhost:3000/download/all and download the
officers.csv. - Run
python data-munging/spd_2021_salary_data.py ~/Downloads/Seattle_Police_Department_Officers.csv data/2022_salaries.csv(the script is for 2021 salaries but unfortunately it's not run for a specific data but point-in-time when the script gets run 😓) I made Make salary 2021 script year agnostic #135 for this just import --salaries-csv /data/2022_salaries.csv "Seattle\ Police\ Department"- Browse for officers and look at their salaries!
Everything there went okay 🙂
Description of Changes
Fixes #126.
Since we use sqlite in our unit tests and sqlite doesn't have a decimal type, sqlalchemy shows a floating-point warning when we work with salaries during testing.
This change fixes the warnings by adding a TypeDecorator to convert currency values to integers by multiplying by 100 when storing values and dividing by 100 when retrieving them. It also changes parts of the salary import code to use Decimal instead of float.
Notes for Deployment
Might be a good idea to test the csv imports a little more before deploying?
Screenshots (if appropriate)
N/A
Tests and linting
I have rebased my changes on
mainjust lintpassesjust testpasses