Actually let T-all start FCPs - #360
Conversation
| type TeamsMap = BTreeMap<TeamLabel, Team>; | ||
| #[derive(Debug, Deserialize, Default)] | ||
| struct TeamsMap { | ||
| #[serde(default, rename = "T-all")] |
There was a problem hiding this comment.
I don't actually know what other repos explicitly use the local config, but to avoid breaking it, I decided to just make it so that T-all is hard-configured to be equivalent to all_members in this case, assuming that nobody has done this.
There was a problem hiding this comment.
I don't think there are any repos that have configuration, if I'm interpreting this search right. I would just hardcode that we allow T-all in all cases for permissions purposes. Not sure if the team labels have influence beyond that.
There was a problem hiding this comment.
To clarify: do you think it's okay that T-all is configured differently? Or would you rather it still work as a regular team?
There was a problem hiding this comment.
Since the alternative is just using the all_members naming, but I figured going with T-all for the toml configuration made a bit more sense. It's arbitrary either way.
There was a problem hiding this comment.
I don't think there is toml configuration anymore? At least I don't see any in most of our repositories. I don't think we need to support per-repository configuration of rfcbot, it should behave the same everywhere.
There was a problem hiding this comment.
In that case, I would be more than happy to just yeet that option from the code entirely if you're okay with the change being a bit larger.
There was a problem hiding this comment.
Yeah, seems easier to simplify.
There was a problem hiding this comment.
Was astonishingly easy, although testing is confusing.
There was a problem hiding this comment.
Okay, tests were confusing but now work.
Mark-Simulacrum
left a comment
Mark-Simulacrum
left a comment
There was a problem hiding this comment.
Can you split out the clippy/rustfmt changes to a separate PR? That'll make this easier to review.
| type TeamsMap = BTreeMap<TeamLabel, Team>; | ||
| #[derive(Debug, Deserialize, Default)] | ||
| struct TeamsMap { | ||
| #[serde(default, rename = "T-all")] |
There was a problem hiding this comment.
I don't think there are any repos that have configuration, if I'm interpreting this search right. I would just hardcode that we allow T-all in all cases for permissions purposes. Not sure if the team labels have influence beyond that.
|
Split out first two commits into #361. (Will rebase this after that is merged.) |
|
(Rebased.) |
| fn subteam_members(issue: &Issue) -> DashResult<Vec<GitHubUser>> { | ||
| // retrieve all of the teams tagged on this issue | ||
| specific_subteam_members(|label| issue.labels.contains(label)) | ||
| specific_subteam_members(|label| issue.labels.iter().any(|l| l == label)) |
There was a problem hiding this comment.
Note: <[T]>::contains only accepts &T, not anything that implements PartialEq.
|
Soft nudge @Mark-Simulacrum to review when you have time. No rush and I figure you just have been busy lately. |
2 participants
Note: to be paired with rust-lang/team#2642 (now merged)
Note that the first two commits are running
cargo fmtandcargo clippy --fix, so, you only want to inspect the last one to see the actual changes.