chore: add manual metrics scripts - #8060
Open
Rathoz wants to merge 7 commits into
Open
Conversation
The luacov config has been dead since #5596 ("wikis top level", 2025-03-10) moved lua/standard/* to lua/wikis/commons/standard/*. Its `include` still pointed at `standard/*`, which no longer matches anything, while `exclude = {'wikis/*'}` covered the entire codebase. With `deletestats = true` on top, every run produced an empty stats file, so no coverage was ever measured. Point include/exclude at the current layout, scoped to lua/wikis/commons, and keep `includeuntestedfiles` so modules no spec loads count as 0% rather than being dropped from the total. Add scripts/metrics/coverage.py to run the suite under coverage and print the summary. --skip-run reuses an existing report, worth having when a full run is ~25s against ~1s for the suite alone. Also un-anchor the luacov entries in .gitignore. Output lands in lua/ because busted runs with `-C lua`, so the root-anchored patterns never matched it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Share of exported functions in lua/wikis/commons whose doc block carries @PARAM or @return. At ~96% it works as a regression guard rather than a growth target: the useful signal is whether new unannotated functions appear. Functions taking no parameters and returning no value need neither tag, so they are reported as exempt and left out of the ratio rather than counted against it. Whether a body returns a value is a heuristic scan to the `end` at the function's own indentation, biased toward asking for an annotation rather than excusing one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Everything in this repo is standardized; what varies is how much a wiki has to override to get the behaviour it wants. Override code is the part that carries maintenance cost, so it is the number worth watching. Declarative data/config and legacy shims are counted separately -- a wiki adding 500 lines of faction data is not the same event as one adding 500 lines of overrides. Classification is by file content, not filename. GetMatchGroupCopyPaste/ wiki.lua, FilterButtons/Config.lua and NotabilityChecker/config.lua all read as declarative from their names and are not. Shares are against all of lua/wikis so "override code is 37.5% of all Lua" is answerable. Also add the missing final newline to Widget/Table2/Row.lua, the only file in the repo without one. It changes no count, but it did make a cat-based cross-check of the commons total read one line short. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 7, 2026
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new metrics logic has correctness issues that can skew results (customization classification and coverage reproducibility) and should be addressed before landing.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a small set of manual, on-demand metrics scripts under scripts/metrics/ to measure Lua coverage, LuaLS annotation coverage, and per-wiki customization size, and updates luacov configuration + gitignore so coverage reporting works against the post-#5596 folder layout.
Changes:
- Add three manual metrics scripts:
coverage.py,annotation_coverage.py,customization_metrics.py. - Fix
lua/.luacovinclude/exclude scope to targetlua/wikis/commonsand retain stats for later inspection/regeneration. - Update
.gitignorefor luacov output location and add the missing final newline inWidget/Table2/Row.lua.
File summaries
| File | Description |
|---|---|
scripts/metrics/customization_metrics.py |
New manual script to classify and report per-wiki override/declarative/legacy LOC and shares. |
scripts/metrics/coverage.py |
New manual script to run the busted suite under luacov and print a coverage summary table. |
scripts/metrics/annotation_coverage.py |
New manual script to compute exported-function doc annotation coverage and output a table (optionally with deltas). |
lua/wikis/commons/Widget/Table2/Row.lua |
Adds missing trailing newline at EOF. |
lua/.luacov |
Fixes luacov scope/exclusions for the new folder layout and retains stats for later regeneration. |
.gitignore |
Adjusts luacov ignore patterns to match outputs produced under lua/. |
Review details
- Files reviewed: 4/6 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
scripts/metrics/ already had three scripts from #7939 and I had not matched their conventions. Bring the new three in line: - numbered as metrics 4-6, following 1-3 - `#!/usr/bin/env python3` - --csv / --no-header with a leading date column, so all six can be appended to a time-series file. This is what the folder is for, and it is the trend history the markdown-table output could not give. - plain aligned tables by default, not markdown. The markdown was for $GITHUB_STEP_SUMMARY, which these no longer write to. - `from pathlib import Path`, type hints Metric 4 now reports loc as metric 1 defines it -- non-blank, non-comment-only lines -- alongside total physical lines. Previously it counted physical lines and called them LOC, so two scripts in the same folder measured the same code differently. Totals now reconcile exactly with metric 1: 1651 files, 203738 lines, 146516 loc. Metric 4 decomposes metric 1 rather than disagreeing with it, and the share column is of loc. Avoided `X | None` annotations: they need python 3.10, and while the workflows pin 3.14 the scripts should still run on an older local interpreter. README covers all six, what each answers, and what they need to run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both from Copilot's review, plus one it did not spot.
Coverage was reporting the previous run. lua/.luacov keeps stats so a report
can be regenerated, but nothing cleared them, so luacov reused whatever was on
disk. Running a single spec with full-suite stats present reported 49.38%
against a true 23.92% -- a 25 point error, and silent. run_suite() now clears
the stats and report first. collect() also fails with a message rather than a
traceback when no report is produced.
The classifier matched the bare `function` keyword, so a comment mentioning
the word could class a data file as override code, and it only learned local
names from `local function name`, missing `local name = function` exported via
`return`. Now matches actual definition forms, strips line comments first, and
records names from both local forms. No file in the repo hits either case
today, so the numbers are unchanged -- this removes latent fragility rather
than fixing a live miscount.
Third one, found while testing the above: export detection used a substring
match, so `local function h` plus `return {a = h()}` counted as exported, and
a short local name could match an unrelated identifier. It is now word
anchored and requires the name not be followed by `(`, which distinguishes
handing a local out (`return F`, `return wrap(F)`) from calling it to build
data (`return {a = h()}`) -- the latter leaves the file declarative, matching
how MainPageLayout data files are already treated.
Metric 4 still reconciles with metric 1: 1651 files, 203738 lines, 146516 loc.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rathoz
changed the title
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The review found a reporting correctness issue in Metric 4 output and CLI inconsistencies versus existing metrics scripts that should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/7 changed files
- Comments generated: 5
- Review effort level: Lite
All from Copilot's second pass. The three scripts took `--no-header` as a plain flag and built a bare ArgumentParser, where the existing three use `description=__doc__` and a `--header` BooleanOptionalAction. Copilot flagged this as inconsistent and it was right -- I had checked the shebang, the metric numbering and --csv when aligning to the folder, but not the parser itself. --help now shows what each script measures, and --no-header still works, from the same declaration. The per-wiki total share added up the already-rounded per-category shares rather than dividing summed loc by total loc. Both give 59.13% today (59.1300 against a true 59.1335), so nothing visible changes, but the wrong one drifts as categories move. Also the singular-subject grammar in the .luacov comment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
annotation_coverage.py currently reports params_total/params_annotated as function-counts rather than parameter-counts, making the params_* metrics misleading.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
scripts/metrics/annotation_coverage.py:100
params_total/params_annotatedare currently counting functions with params (incrementing by 1 per function), but the field names andparams_pctsuggest these are parameter-level totals. As written, a function with 5 parameters counts the same as one with 1, which makes theparams_*metrics misleading.
- Files reviewed: 5/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
Per Copilot: params_total and params_annotated incremented once per function rather than once per parameter, so a five-parameter signature counted the same as a one-parameter one while the field names said otherwise. This was a regression from the move to the house table format. The markdown version labelled the row "Functions with all params annotated", which was accurate; deriving labels from field names dropped that and left the table printing "params total 2046" for a count of functions. Now counts individual parameters, which is what the names imply: 2991 of 3148 annotated, 95.01%, against the 95.60% the function-level count reported. Clamped per function, since a doc block can carry more @PARAM than the signature takes. Worth doing now rather than later: no time series exists yet, so redefining these columns costs nothing. After the first append it would break comparability. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Member
Author
|
On the latest review — the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Three more scripts in
scripts/metrics/, following the three from #7939. Run by hand;--csvappends to a time-series file like the existing ones.Metric 4 — per-wiki customization. How much override code exists, the part that carries maintenance cost. Declarative data/config and legacy shims counted separately. Categories come from file content, not filename.
locmatches metric 1 (non-blank, non-comment-only), so this decomposes metric 1 rather than disagreeing with it — the totals reconcile exactly at 1651 files / 203738 lines / 146516 loc.Metric 5 — annotation coverage. Exported functions in commons carrying
@param/@return. At ~96% it's a regression guard. Functions taking no params and returning nothing are exempt, not counted as gaps.Metric 6 — test coverage. Needs a config fix too:
lua/.luacovhas been dead since #5596 (2025-03-10) movedlua/standard/*→lua/wikis/commons/standard/*, so itsincludematched nothing andexcludecovered everything. Coverage was never actually measured.Also: a README for the folder, un-anchored
luacov.*gitignore entries (output lands inlua/, so they never matched), and the missing final newline onWidget/Table2/Row.lua.Replaces #7862, #7863 and #7864. Review feedback from those (@ElectricalBoy, @hjpalpha) is folded in.
How it was tested
coverage.pyend to end on real Lua 5.1.5: 808 pass / 0 fail--csv,--no-header,--base,--skip-run, missing-tool and missing-directory paths all checkedruff check+format