-
Notifications
You must be signed in to change notification settings - Fork 6
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
refactor: implement result typing #24
base: main
Are you sure you want to change the base?
Conversation
ee36109
to
9bd3408
Compare
@martinbrose I'll need your input on which strategy to use for handling megabits, as listed in the todo section. |
Thanks @Nytelife26, I'll have a look by the weekend at the latest. Hopefully earlier. |
This also fixes a bug where --json would omit some entries.
fce3d7c
to
5e83d66
Compare
I took the initiative to evaluate each strategy for handling megabits, and figured the last one would be the simplest and least surprising (implementation wise). {
"megabits": true,
"meta": {...},
"tests": {
"latency": {"value": 15.61, ...},
"jitter": {"value": 7.19, ...},
"100kB_down": {"value": 41.33, ...},
"100kB_up": {"value": 23.01, ...}
},
"90th_percentile_down": {"value": 96.43, ...},
"90th_percentile_up": {"value": 24.67, ...}
} this is also reported by the CLI, now with keys that reflect the dict structure:
|
quick follow up - @martinbrose, how's your schedule looking? i'm excited to continue working on martinbrose/cloudflare-speedtest-exporter#54 after we land this. |
Fixes #23.
It came to my attention when I was working on martinbrose/cloudflare-speedtest-exporter#54 that it would be useful for end users to have a properly typed results class, instead of either having to work with a loosely structured dict or collect it into their own named tuples.
This also fixes a bug introduced in #19 where --json would omit some entries. See below:
I would recommend #21 be merged first, so this can serve as its successor, and possibly #22.
TODO:
_mbps
keys alongside_bps
(bps, mbps)