-
Notifications
You must be signed in to change notification settings - Fork 779
fix(gain): use weighted savings rate in per-command stats #891
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
base: develop
Are you sure you want to change the base?
Changes from all commits
8b5015d
981f561
b3ab873
5de6ab2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -221,7 +221,12 @@ pub struct MonthStats { | |||||||
| pub avg_time_ms: u64, | ||||||||
| } | ||||||||
|
|
||||||||
| /// Type alias for command statistics tuple: (command, count, saved_tokens, avg_savings_pct, avg_time_ms) | ||||||||
| /// Type alias for command statistics tuple: (command, count, saved_tokens, weighted_savings_rate, avg_time_ms) | ||||||||
| /// | ||||||||
| /// # Warning | ||||||||
| /// The 4th field is a **weighted** savings rate: `SUM(saved_tokens) * 100.0 / SUM(input_tokens)`. | ||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Seems more idiomatic to express a percentage |
||||||||
| /// Do NOT aggregate this column with `AVG()` — that would produce an unweighted mean that | ||||||||
| /// under-weights high-volume commands. Always use `SUM(saved) / SUM(input)` instead. | ||||||||
|
||||||||
| /// under-weights high-volume commands. Always use `SUM(saved) / SUM(input)` instead. | |
| /// under-weights high-volume commands. Always recompute it as | |
| /// `SUM(saved_tokens) * 100.0 / SUM(input_tokens)` instead. |
Copilot
AI
Mar 28, 2026
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 regression test comment’s expected weighted value looks off: (10 + 95_000) * 100 / (100 + 100_000) is ~94.9%, not ~94.99%. Consider correcting the comment (or compute the expected rate and assert within a small epsilon) so the test documents the math accurately.
| // Weighted rate = (10 + 95_000) * 100.0 / (100 + 100_000) ≈ 94.99% | |
| // Weighted rate = (10 + 95_000) * 100.0 / (100 + 100_000) ≈ 94.9% |
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.
Exact final value is 94.9150849150849150 so yeah much more near 94.9 than 94.99
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,10 +15,10 @@ pub struct RtkRule { | |
|
|
||
| // Patterns ordered to match RULES indices exactly. | ||
| pub const PATTERNS: &[&str] = &[ | ||
| r"^git\s+(?:-[Cc]\s+\S+\s+)*(status|log|diff|show|add|commit|push|pull|branch|fetch|stash|worktree)", | ||
| r"^git\s+(?:-[Cc]\s+\S+\s+)*(status|log|diff|show|add|commit|push|pull|branch|fetch|stash|worktree|checkout|merge|rebase)", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using alphabetically sorted list would ease maintenance |
||
| r"^gh\s+(pr|issue|run|repo|api|release)", | ||
| r"^cargo\s+(build|test|clippy|check|fmt|install)", | ||
| r"^pnpm\s+(list|ls|outdated|install)", | ||
| r"^cargo\s+(build|test|clippy|check|fmt|install|run|publish)", | ||
| r"^pnpm\s+(list|ls|outdated|install|build|exec)", | ||
|
Comment on lines
+18
to
+21
|
||
| r"^npm\s+(run|exec)", | ||
| r"^npx\s+", | ||
| r"^(cat|head|tail)\s+", | ||
|
|
@@ -102,7 +102,11 @@ pub const RULES: &[RtkRule] = &[ | |
| ("add", 59.0), | ||
| ("commit", 59.0), | ||
| ], | ||
| subcmd_status: &[], | ||
| subcmd_status: &[ | ||
| ("checkout", RtkStatus::Passthrough), | ||
| ("merge", RtkStatus::Passthrough), | ||
| ("rebase", RtkStatus::Passthrough), | ||
| ], | ||
| }, | ||
| RtkRule { | ||
| rtk_cmd: "rtk gh", | ||
|
|
@@ -118,15 +122,22 @@ pub const RULES: &[RtkRule] = &[ | |
| category: "Cargo", | ||
| savings_pct: 80.0, | ||
| subcmd_savings: &[("test", 90.0), ("check", 80.0)], | ||
| subcmd_status: &[("fmt", RtkStatus::Passthrough)], | ||
| subcmd_status: &[ | ||
| ("fmt", RtkStatus::Passthrough), | ||
| ("run", RtkStatus::Passthrough), | ||
| ("publish", RtkStatus::Passthrough), | ||
| ], | ||
| }, | ||
| RtkRule { | ||
| rtk_cmd: "rtk pnpm", | ||
| rewrite_prefixes: &["pnpm"], | ||
| category: "PackageManager", | ||
| savings_pct: 80.0, | ||
| subcmd_savings: &[], | ||
| subcmd_status: &[], | ||
| subcmd_status: &[ | ||
| ("build", RtkStatus::Passthrough), | ||
| ("exec", RtkStatus::Passthrough), | ||
| ], | ||
| }, | ||
| RtkRule { | ||
| rtk_cmd: "rtk npm", | ||
|
|
||
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.
PR description is centered on
gain’s weighted savings calculation, but this change also updates general repository guidance (supported ecosystems list). Please either mention this documentation update in the PR description or move it to a separate doc-only PR to keep the change set focused.