ci: show newest binary-size revisions first - #41
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Newest-first benchmark run ordering
The change flips run ordering to newest-first and is implemented consistently across the Python enrichment (order_runs), the dashboard (latestRun, chartRuns, pagination init), the README, and the tests.
Verified correct:
order_runssorts both main runs (byllgoMainIndex) and other runs (by timestamp) withreverse=True, correctly relying on--main-historybeing oldest-first (positions enumerated 1=oldest → N=newest).latestRun()now returnsruns[0], consistent with newest-first.chartRuns()slices the newest N and.reverse()s on a fresh copy — the sourcestate.index.runsis never mutated — so trend charts still read chronologically left-to-right.- Removing the
state.page = ceil(...)init is correct: page 1 (state.pagedefault) is now the newest page. - README and tests accurately match the new behavior.
No correctness, security, or performance issues found. Two minor readability nits inline.
| function chartRuns() { | ||
| const limit = Number(dom.historyRange.value); | ||
| return limit > 0 ? state.index.runs.slice(-limit) : state.index.runs.slice(); | ||
| const newest = limit > 0 ? state.index.runs.slice(0, limit) : state.index.runs.slice(); |
There was a problem hiding this comment.
Nit: the variable is named newest, but it's returned reversed to oldest-first on the next line. A name like recent or selected would read less contradictorily, since the returned array is chronological (oldest→newest), not newest-first. Behavior is correct.
|
|
||
| main_runs = [run for run in runs if is_main_run(run)] | ||
| other_runs = [run for run in runs if not is_main_run(run)] | ||
| main_runs.sort(key=lambda run: run["llgoMainIndex"], reverse=True) |
There was a problem hiding this comment.
Nit: the newest-first result depends on main_history being passed oldest-first (enumerated 1=oldest → N=newest, then sorted reverse=True). That invariant is only documented in the argparse help. A one-line comment here noting main_history is expected oldest-first would keep the intent local to where it's consumed.
1 participant
Summary
Validation
python3 -m unittest ci/llgo-size/test_enrich_pull_requests.pygit diff --check