Skip to content

remove duplicate code in validation - #2701

Merged
ubiratansoares merged 1 commit into
mainfrom
refactor-validation
Aug 28, 2026
Merged

remove duplicate code in validation#2701
ubiratansoares merged 1 commit into
mainfrom
refactor-validation

Conversation

@marcoieni

@marcoieni marcoieni commented Aug 21, 2026

Copy link
Copy Markdown
Member

I extracted a variable.

AI disclosure

I used GPT5.6-Sol with the codex harness to generate this change. I reviewed its output and changed it where necessary.

@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown
Dry-run check results

[WARN  rust_team::sync] sync-team is running in dry mode, no changes will be applied.
[INFO  rust_team::sync] synchronizing crates-io
[INFO  rust_team::sync] synchronizing github
[INFO  rust_team::sync] 💻 Team Diffs:
    ➕ Creating team:
      Org: rust-lang
      Name: rust-timer
      Description: Managed by the rust-lang/team repository.
      Privacy: closed
      Members:
        rust-timer: member

Comment thread src/validate.rs Outdated
errors.push(e.to_string());
}

let alumni = team

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Cloning iterators is a bit icky, I usually create a closure instead (let alumni = || ...) and then call it repeatedly.

@steffahn

Copy link
Copy Markdown
Member

Alternatively, to reduce some of the duplication already, the "leads + alumni" check isn't really necessary in the first place, given that we also check leads is a subset of members.

@steffahn

Copy link
Copy Markdown
Member

Also actually, with the "members + alumni" check in place, the individual checks on members and on alumni become redundant, because those also create duplicates in members + alumni combined

@steffahn

Copy link
Copy Markdown
Member

so you just need 2 checks total; one on members + alumni and one on leads. And no duplicate code is left at all

@Kobzol

Kobzol commented Aug 21, 2026

Copy link
Copy Markdown
Member

Yeah, but this situation is so rare that I didn't want to bother with it. And if we only check members + alumni together, then the error message will be misleading (it would have to be generalized to something like "there is duplication either amongst members or members + alumni").

@steffahn

Copy link
Copy Markdown
Member

This is what I thought it can look like: #2702

@marcoieni
marcoieni force-pushed the refactor-validation branch from f55d02d to 23c40d2 Compare August 21, 2026 14:23
@rustbot

rustbot commented Aug 21, 2026

Copy link
Copy Markdown

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@marcoieni

marcoieni commented Aug 21, 2026

Copy link
Copy Markdown
Member Author

Thanks both!
I implemented Kobzol's suggestion. I'll let the reviewer decide which approach they prefer. If they prefer #2702, feel free to close this PR. 👍

@jieyouxu jieyouxu added needs-infra-admin-review This change requires one of the `infra-admins` to review. S-waiting-on-review Status: waiting on review from a team/WG/PG lead, an infra-admin, and/or a team-repo-admin. labels Aug 28, 2026
@ubiratansoares
ubiratansoares added this pull request to the merge queue Aug 28, 2026
Merged via the queue into main with commit 1b5a8b3 Aug 28, 2026
4 checks passed
@ubiratansoares
ubiratansoares deleted the refactor-validation branch August 28, 2026 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-infra-admin-review This change requires one of the `infra-admins` to review. S-waiting-on-review Status: waiting on review from a team/WG/PG lead, an infra-admin, and/or a team-repo-admin.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants