-
Notifications
You must be signed in to change notification settings - Fork 373
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
Improve size tracking table #3117
Conversation
scripts/ci/sizes.py
Outdated
|
||
min_change = previous * (threshold / 100) |
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.
The units here confuse me. What unit does threshold
have? If it is in percents, why not just compare abs(change_pct) >= threshold
? Again: proper names help a lot.
I prefer to not use percentages except for ui:s (i.e. multiply with 100 during the formatting). It usually ends up with far less 100
in the code (and less bugs). _ratio
or _fraction
is a pretty good suffix for such numbers. Percentages in arguments have the problem of the unit being invisible. --threshold 0.05
is usually clearer than --threshold 5
(5 kiB?)
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.
If you're about to use the script, you're going to run sizes.py --help
first, and the help text for threshold
says N%
, so --threshold 5
means 5%
. As a user, that seems marginally better than 0.05
, because the table also outputs percentages. I tell the script I want to see differences of more than 5% and I see all the differences which were more than 5%, no mental math required, however little it may be.
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.
That's much clearer to read now - thanks 🙏
comparison=$(python3 scripts/ci/sizes.py compare --threshold=5 "/tmp/prev.json" "/tmp/data.json") | ||
comparison=$( | ||
python3 scripts/ci/sizes.py compare \ | ||
--threshold=5 \ |
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.
I understand that sizes.py --help
explains the unit of threshold
, but that helps very little when reading this file 🤷♂️
Not a blocker, just trying to explain my reasoning a bit further.
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.
I agree, but I also think that 0.05
is as ambigous as 5
if there's no unit :/
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.
Maybe - but 0.05
is less likely to be of unit Bytes at least :)
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.
--threshold-percent
would be very clear in any case
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.
We can do crimes instead:
comparison=$(
python3 scripts/ci/sizes.py compare \
--threshold=5% \
"prev.json" "data.json"
)
The %
is optional
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.
Haha, now we're talking 😆
What
main
Checklist