Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: evaluate integer percentage #114

Merged
merged 2 commits into from
Oct 1, 2024
Merged

fix: evaluate integer percentage #114

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0b32dc1
fix: evaluate integer percentage
rayzhou-bit Oct 1, 2024
6476c25
fix: test
rayzhou-bit Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions calc/calc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to still see SI extensions mentioned and some example would also be helpful in the comment.

"""
if text[-1] in SUFFIXES:
return float(text[:-1]) * SUFFIXES[text[-1]]
Expand All @@ -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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return super_float("".join(parse_result))

return int("".join(parse_result))
Expand Down
Loading