Skip to content

ci: clarify parser diff comments#6038

Merged
matthewevans merged 1 commit into
mainfrom
ship/ci-clarify-parser-diff-comments
Jul 16, 2026
Merged

ci: clarify parser diff comments#6038
matthewevans merged 1 commit into
mainfrom
ship/ci-clarify-parser-diff-comments

Conversation

@matthewevans

Copy link
Copy Markdown
Member

No description provided.

@matthewevans
matthewevans enabled auto-merge July 16, 2026 19:14
@matthewevans
matthewevans disabled auto-merge July 16, 2026 19:16
@matthewevans
matthewevans merged commit 63e95a4 into main Jul 16, 2026
12 checks passed
@matthewevans
matthewevans deleted the ship/ci-clarify-parser-diff-comments branch July 16, 2026 19:16

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces structured grouping and visual markers for different change kinds in the coverage report markdown output. It adds helper methods to the ChangeKind enum, refactors the rendering logic into a new function render_cluster_sections, and includes comprehensive regression tests to verify the grouping and marker logic. The reviewer suggests an optimization to avoid double iteration in the rendering function to improve readability and adhere to idiomatic Rust practices.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +535 to +574
for kind in CHANGE_KIND_ORDER {
let signature_count = clusters.iter().filter(|c| c.kind == kind).count();
if signature_count == 0 {
continue;
}
let signature_label = if signature_count == 1 {
"signature"
} else {
"signatures"
};
let _ = writeln!(
s,
"#### {} ({} {})\n",
kind.section_heading(),
signature_count,
signature_label,
);

for c in clusters.iter().filter(|c| c.kind == kind) {
let card_label = if c.cards.len() == 1 { "card" } else { "cards" };
let _ = writeln!(
s,
"- **{} {}** · {} {}",
c.cards.len(),
card_label,
c.kind.marker(),
describe(c),
);
if show_cards {
let cards: Vec<&str> = c.cards.iter().take(3).map(String::as_str).collect();
let more = c.cards.len().saturating_sub(cards.len());
let _ = write!(s, " - Affected (first 3): {}", cards.join(", "));
if more > 0 {
let _ = write!(s, " (+{more} more)");
}
s.push('\n');
}
}
s.push('\n');
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This function iterates over the clusters slice twice for each ChangeKind: once to count the items and again to render them. While not a major performance issue given the expected size of clusters, this can be made more readable and idiomatic by collecting the relevant clusters for each kind into a Vec first. This avoids the repeated filtering and makes the intent clearer.

    for kind in CHANGE_KIND_ORDER {
        let kind_clusters: Vec<_> = clusters.iter().filter(|c| c.kind == kind).collect();
        if kind_clusters.is_empty() {
            continue;
        }

        let signature_count = kind_clusters.len();
        let signature_label = if signature_count == 1 {
            "signature"
        } else {
            "signatures"
        };
        let _ = writeln!(
            s,
            "#### {} ({} {})
",
            kind.section_heading(),
            signature_count,
            signature_label,
        );

        for c in kind_clusters {
            let card_label = if c.cards.len() == 1 { "card" } else { "cards" };
            let _ = writeln!(
                s,
                "- **{} {}** · {} {}",
                c.cards.len(),
                card_label,
                c.kind.marker(),
                describe(c),
            );
            if show_cards {
                let cards: Vec<&str> = c.cards.iter().take(3).map(String::as_str).collect();
                let more = c.cards.len().saturating_sub(cards.len());
                let _ = write!(s, "  - Affected (first 3): {}", cards.join(", "));
                if more > 0 {
                    let _ = write!(s, " (+{more} more)");
                }
                s.push('\n');
            }
        }
        s.push('\n');
    }
References
  1. The style guide (pillar 1) emphasizes using idiomatic Rust and standard library idioms to their fullest. The suggested change improves efficiency and readability by avoiding a double iteration pattern over the same data. (link)

@github-actions

Copy link
Copy Markdown

Parse changes introduced by this PR

✓ No card-parse changes detected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant