[code-simplifier] Simplify detection_comparison.py: extract helper functions for casting and legend handling - #58140
Conversation
…nd legend combiner Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Ponytail Reviewer completed successfully! Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review. Reviewed PR #58140 and submitted a COMMENT review; no separate GitHub write beyond the review was needed.
|
There was a problem hiding this comment.
🔵 Needs a closer look
The modified script is protected and requires explicit human review.
Pull request overview
Refactors chart generation without changing metric parsing or legend output.
Changes:
- Centralizes integer and float metric conversion.
- Extracts reusable legend-entry aggregation.
File summaries
| File | Description |
|---|---|
.github/scripts/detection_comparison.py |
Extracts metric-casting and legend-combination helpers. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Two small simplifications look worth cutting:
- L30: shrink: local get_int/get_float helpers for six one-line casts. Inline the direct
int(m.get(...))/float(m.get(...))calls. - L89: shrink: combined_legend_entries() wrapper around one two-axis legend merge. Inline the two legend fetches and keep the legend call direct.
net: -8 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #58140 · codex · mai10 · 3.71 AIC · ⌖ 16.1 AIC · ⊞ 13.5K
Comment /ponytail to run again
| regular_failure = int(m.get("regular_failure_count", 0)) | ||
| detection_failure = int(m.get("detection_failure_count", 0)) | ||
| misconfigured_count = int(m.get("misconfigured_count", 0)) | ||
| def get_int(key: str, default: int = 0) -> int: |
There was a problem hiding this comment.
L30: shrink: local get_int/get_float helpers for six one-line casts. Inline the direct int(m.get(...))/float(m.get(...)) calls.
| handles1, labels1 = ax1.get_legend_handles_labels() | ||
| handles2, labels2 = ax2.get_legend_handles_labels() | ||
| ax1.legend(handles1 + handles2, labels1 + labels2, loc="upper right", fontsize=10) | ||
| def combined_legend_entries(*axes): |
There was a problem hiding this comment.
L89: shrink: combined_legend_entries() wrapper around one two-axis legend merge. Inline the two legend fetches and keep the legend call direct.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design lightly — this is a small, low-risk extractive refactor with no behavior changes.
📋 Key Themes & Highlights
Positive Highlights
- ✅
get_int/get_floathelpers cleanly remove 7x duplicated cast-and-default logic, matching existing naming style. - ✅
combined_legend_entries(*axes)generalizes the two-axis handle/label concatenation without changing output order or content. - ✅ No new abstractions leak outside
main(); scope stays local and easy to follow.
No actionable issues found. py_compile validation was performed per the PR description; no test suite exists for this script.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 15.9 AIC · ⌖ 13.5 AIC · ⊞ 10.3K
Comment /matt to run again
There was a problem hiding this comment.
Reviewed via Impeccable-style pass. This is a pure Python script refactor (no UI), so Impeccable UI modes (audit/critique/harden/distill/extract/clarify) do not directly apply; treated as a general correctness/maintainability review instead.
The change is a small, behavior-preserving extraction:
get_int/get_floathelpers replace 7x duplicatedint(m.get(key, default))casts — correct, same keys/defaults.combined_legend_entries(*axes)replaces manualhandles1 + handles2concatenation — correct, same order/content.
No correctness, security, or reliability issues found. No blocking changes needed.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 13 AIC · ⌖ 14.2 AIC · ⊞ 8.3K
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Verdict
No actionable blocking issues found in the changed lines.
Why I'm not blocking this
The patch is a straight helper extraction in .github/scripts/detection_comparison.py: the new get_int/get_float wrappers preserve the same keys and defaults, and combined_legend_entries(ax1, ax2) preserves the original legend ordering and contents. I also discarded the sub-agent result because it referred to a different file outside this diff.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 14.7 AIC · ⌖ 7.31 AIC · ⊞ 23.5K
Comment /review to run again
|
🎉 This pull request is included in a new release. Release: |
Caution
Protected files were modified in this change.
This pull request is in
request_reviewmode and requires explicit human scrutiny before merge.Protected files:
.github/scripts/detection_comparison.pySummary
Extractive, behavior-preserving simplifications to
.github/scripts/detection_comparison.py, identified via the recent-change scan (candidate file list) and scored by the simplification scout.Changes
int(m.get(key, default))/float(m.get(key, default))patterns into smallget_int/get_floathelpers, removing 7x duplicated cast-and-default logic.ax1/ax2) into acombined_legend_entries(*axes)helper for clarity, replacing the manualhandles1 + handles2/labels1 + labels2pattern.No behavior changes: same JSON keys/defaults are read, same legend content/order is produced, same chart output.
Source references
recent-context.json,source-files.json,history-summary.json) rather than re-querying GitHub, per the token-efficiency guidance inhistory-summary.json'sdeterministic_candidates.Validation
python3 -m py_compile .github/scripts/detection_comparison.py— passes.make test-unit,make lint,make buildare Go-focused and unaffected by this change); confirmed no Makefile/workflow referencesdetection_comparison.pyas a test/lint entry point.Notes
Other candidate files evaluated (e.g.
add_comment.cjs,add_labels.cjs) were larger, higher-risk targets flagged by scope-filter for future passes; this run limited scope to the lowest-risk, fully-validated change to stay within the token/run budget.