-
-
Notifications
You must be signed in to change notification settings - Fork 32k
gh-109373: Store metadata required for pystats comparison in the json #109374
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just one suggestion to maybe improve the funny ratio_to_float
function. It's minor, though, so I'll merge tomorrow regardless.
Tools/scripts/summarize_stats.py
Outdated
def ratio_to_float(s): | ||
""" | ||
The inverse of format_ratio. | ||
""" | ||
if s == "": | ||
return 0.0 | ||
else: | ||
return float(s[:-1]) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This confused me, because it's not the inverse, right? It just turns "4.2%"
into 4.2
(which feels wrong to me since I'd expect .042
... I know that this is what the old code does, and it's only used as a sort key, so I guess either is fine).
So I'd suggest changing the name and docstring, but I honestly can't think of a good name for this operation...
At least, if this is gonna be its own function, maybe assert s.endswith("%")
so it's clear what this function is doing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I renamed the function and clarified in the docstring.
@@ -179,6 +192,16 @@ def gather_stats(input): | |||
value = int(value) | |||
stats[key] += value | |||
stats['__nfiles__'] += 1 | |||
|
|||
import opcode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, why was this import moved here? Just to reduce the temptation to use it elsewhere in the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly. Any use of opcode
, dis
or other sorts of bytecode introspection things on the "compare two results" path could lead to a subtle, silent error. I'll add a comment.
Uh oh!
There was an error while loading. Please reload this page.