fix: evaluate integer percentage #114
Conversation
There was a problem hiding this comment.
Nice work identifying the bug! The library does not seem straightforward to me, it seems a bit overengineered to go through all this trouble just to parse a percentage number ^^ so it's great you figured this out!
I'm not sure how you tested this, but it's a library, right? Typically we would follow Test-Driven Development practices to make sure that the library behaves correctly, since we don't test libraries manually so much.
Generally I think doing red-green testing on this would be a good approach: first writing a failing test that properly catches the bug you found, and only afterwards writing the code that fixes it until the test becomes green.
You'd also want to think about every conceivable edge case, but I can't come up with any relevant edge case here, so I think it's just a matter of writing that new test and we should be good.
Another thing to consider is why this bug occurred. I mean this is a very standard operation, right? Converting a string that may include percentage sign into a float. In my opinion this should not have occurred since in a mathematical library there are very defined operations.
Can we answer these questions:
- is this library sufficiently tested? I mean are we confident the tests will catch whatever we throw at it and this was just one unfortunate exception? Or are the tests just not sufficient and we should reevaluate them?
- did anything about the way the code is written make this bug likely to happen? Why is it there is a bug in such an apparently straightforward operation? What could we improve to avoid bugs like this going forward?
calc/calc.py
Outdated
| @@ -128,7 +128,7 @@ def eval_number(parse_result): | |||
| Calls super_float above. | |||
| """ | |||
| for item in parse_result: | |||
| if "." in item or "e" in item or "E" in item: | |||
| if "." in item or "e" in item or "E" in item or parse_result[-1] in SUFFIXES: | |||
There was a problem hiding this comment.
It may just be me, but this doesn't feel exactly right. First of all, it's code duplication with line 115 that I think shows there's something about the code design that's not ideal, and secondly I don't like that we have a random-looking set of conditions which result in this being evaluated as a float, and then a default case that's int. It just intuitively does not look confidence-inspiring to me. Do you have any ideas?
There was a problem hiding this comment.
why not "%" in item? Not a suggestion, I genuinely try to understand
I also don't think suffixes necessarily only contains percentage sign, so the check here doesn't seem very logical.
And... is "e" and "%" the only SI extensions that could be relevant? What if someone adds another extension?
Definitely some code smells for me.
There was a problem hiding this comment.
Looking at this again, it looks like previously the code was to simply always apply super_float and was changed to have this conditional in a python 3.11 support PR. The idea behind the condition is to apply super_float if the number has a decimal, has scientific notation or a suffix (missing before this PR). I agree it's not ideal and I'll think about a more elegant solution.
| @@ -112,7 +112,7 @@ def lower_dict(input_dict): | |||
|
|
|||
| def super_float(text): | |||
| """ | |||
| Like float, but with SI extensions. 1k goes to 1000. | |||
| Evaluate suffixes if applicable and apply float. | |||
There was a problem hiding this comment.
I would like to still see SI extensions mentioned and some example would also be helpful in the comment.
|
I have to say I find it quite suspicious that we need to write our own, obviously buggy, library to convert something like a percentage string into a float. Isn't there a more properly tested and more reliable python package that does that that we know is good? |
|
@jesperhodge I did test the code through tests on The way the code is written here, I'm expecting many more I am very confident in this fix because the specific issue is that |
|
Interesting note that unit test for -70% = -0.7 does not work while other numbers do.
|
Currently, an integer percentage answer like "1%" does not evaluate as a number while "1.0%" does. This is due to the % suffix not being evaluated when there is no "." in the answer. This PR fixes this issue.