chore : validate no duplicate team members #1918

[vivekjami]

Added HashSet to track members in validate_team_members. Validation now fails if duplicate found in same team like rylev in mods-venue.toml. Fixes #1918

[Kobzol]

Looks good, thanks!

[github-actions]

Dry-run check results

[WARN  sync_team] sync-team is running in dry mode, no changes will be applied.
[INFO  sync_team] synchronizing github

[marcoieni]

Thanks for the PR 🚀
A few things!

commit history

I noticed you updated your branch.

Normally this doesn't matter but in rust-lang we usually merge the commit (we don't squash), so we don't want those commits to get in the commit history.

Can you squash your commits? I can do it for you if you've never done it and show how I do it later.

does it work?

Also, have you tested this? I tried adding duplicate team members and ran cargo run -- check but the check is succeeding.

[Kobzol]

Aah, lol, I didn't realize that team.members() already returns a HashSet 🤦

[vivekjami]

@marcoieni might need some git knowledge , no the code is properly running in my system(i meant the new code) , but i guess everything messed up in the pr.

[vivekjami]

@Kobzol this is the part i got a bit confused , and endedup checking the already hashed set, that gave no results, so now i tried to take the member directly and that worked

[vivekjami]

if anyone wants to try this is the code just change the function on line 223 or some thing with this function :

/// Ensure team members are real people and not duplicated
fn validate_team_members(data: &Data, errors: &mut Vec<String>) {
    wrapper(data.teams(), errors, |team, errors| {
        // Check for duplicate members in explicit_members
        let mut seen = HashSet::new();
        for member in team.explicit_members() {
            if !seen.insert(&member.github) {
                bail!(
                    "person `{}` is duplicated in team `{}`",
                    member.github,
                    team.name()
                );
            }
        }
        
        // Existing validation: ensure members are real people
        wrapper(team.members(data)?.iter(), errors, |member, _| {
            if data.person(member).is_none() {
                bail!(
                    "person `{}` is member of team `{}` but doesn't exist",
                    member,
                    team.name()
                );
            }
            Ok(())
        });
        Ok(())
    });
}

[Kivooeo]

I would be able to test in 30 minutes

[Kivooeo]

Yes, it's working as expected

By the way, I had also considered a solution for this issue (before this PR), which involved creating a new function (and a new check, accordingly). That approach worked too, so I'm not sure which one is better from a code design perspective

(in fact, neither mine or their solution checks alumni lists for duplication, I'm not sure, do we want to check only member lists?)

[vivekjami]

@Kivooeo I had the same doubt while writing a check for only members 😅, but I only stick to the pr as I just got here 😂

[marcoieni]

I think we should check alumni. Also a separate function seems more clean! Great suggestions Kivooeo, thanks!

[Kivooeo]

Here is the best I could came up with, feel free to reuse for this PR

https://github.com/rust-lang/team/compare/master...Kivooeo:add-check?expand=1

Works well except one case

leads = ["carols10cents", "carols10cents", "chriskrycho"]
members = [
    "carols10cents",
    "chriskrycho",
    "chriskrycho",
]

in this case it will report only leads, because it's not checking further and I don't want to ruin code expressiveness to make it work, it has a good vibe

[ERROR rust_team::validate] validation error: team `book` has duplicate leads: carols10cents
[ERROR rust_team::validate] validation error: team `compiler-fcp` has duplicate leads: cjgillot
[ERROR rust_team::validate] validation error: team `wg-compiler-performance` has duplicate members: nnethercote, panstromek, Zoxc
[ERROR rust_team::validate] validation error: team `wg-mir-opt` has duplicate alumni: spastorino

error looks like this

(the reason I ended up with adding new explicit_leads method is because original team.leads already have sorted from duplicates list, so I could not work with that one to search for duplicates)