Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't ping everyone on rebase mistakes #1672

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
74 −1
Open

Don't ping everyone on rebase mistakes #1672

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 105,9 @@ pub(crate) struct NoteConfig {
}

#[derive(PartialEq, Eq, Debug, serde::Deserialize)]
#[serde(rename_all = "kebab-case")]
pub(crate) struct MentionsConfig {
pub(crate) bors_commit_message: Option<String>,
#[serde(flatten)]
pub(crate) paths: HashMap<String, MentionsPathConfig>,
}
Expand Down Expand Up @@ -398,4 400,46 @@ mod tests {
}
);
}

#[test]
fn mentions_config() {
let config = r#"
[mentions."some_other_path"]
message = "foo"
cc = ["@someone"]

[mentions]
bors-commit-message = "has bors commit"
"#;
let config = toml::from_str::<Config>(config).unwrap().mentions.unwrap();
assert!(config.paths.len() == 1);
assert_eq!(
config.bors_commit_message.as_deref(),
Some("has bors commit")
);

let config = r#"
[mentions]
bors-commit-message = "has bors commit"

[mentions."some_other_path"]
message = "foo"
cc = ["@someone"]
"#;
let config = toml::from_str::<Config>(config).unwrap().mentions.unwrap();
assert!(config.paths.len() == 1);
assert_eq!(
config.bors_commit_message.as_deref(),
Some("has bors commit")
Copy link

Choose a reason for hiding this comment

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

How could this middle test possibly fail?

This test should be over the overall output, not the config system

);

let config = r#"
[mentions."some_other_path"]
message = "foo"
cc = ["@someone"]
"#;
let config = toml::from_str::<Config>(config).unwrap().mentions.unwrap();
assert!(config.paths.len() == 1);
assert_eq!(config.bors_commit_message, None);
}
}
1 change: 1 addition & 0 deletions src/github.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1863,6 1863,7 @@ pub struct GitCommit {

#[derive(Debug, serde::Deserialize)]
pub struct GitUser {
pub name: String,
pub date: DateTime<FixedOffset>,
}

Expand Down
30 changes: 29 additions & 1 deletion src/handlers/mentions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 17,7 @@ use tracing as log;
const MENTIONS_KEY: &str = "mentions";

pub(super) struct MentionsInput {
has_bors_commit: bool,
paths: Vec<String>,
}

Expand Down Expand Up @@ -50,6 51,17 @@ pub(super) async fn parse_input(
return Ok(None);
}

let has_bors_commit = event.action == IssuesAction::Opened
&& config.bors_commit_message.is_some()
&& event
.issue
.commits(&ctx.github)
.await
.map_err(|e| log::error!("failed to fetch commits: {:?}", e))
.unwrap_or_default()
.into_iter()
.any(|commit| commit.commit.author.name == "bors");
Comment on lines 54 to 63
Copy link
Member

Choose a reason for hiding this comment

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

Does this even work for problematic rebases? As someone pointed out on Zulip, https://github.com/rust-lang/rust/pull/103918/commits does not have any bors-authored commits.


if let Some(diff) = event
.issue
.diff(&ctx.github)
Expand Down Expand Up @@ -78,7 90,10 @@ pub(super) async fn parse_input(
.map(|(key, _mention)| key.to_string())
.collect();
if !to_mention.is_empty() {
return Ok(Some(MentionsInput { paths: to_mention }));
return Ok(Some(MentionsInput {
has_bors_commit,
paths: to_mention,
}));
}
}
Ok(None)
Expand All @@ -95,6 110,14 @@ pub(super) async fn handle_input(
IssueData::load(&mut client, &event.issue, MENTIONS_KEY).await?;
// Build the message to post to the issue.
let mut result = String::new();
if input.has_bors_commit {
write!(
result,
"{}\n",
config.bors_commit_message.as_deref().unwrap()
)
.unwrap();
}
for to_mention in &input.paths {
if state.data.paths.iter().any(|p| p == to_mention) {
// Avoid duplicate mentions.
Expand All @@ -109,6 132,11 @@ pub(super) async fn handle_input(
None => write!(result, "Some changes occurred in {to_mention}").unwrap(),
}
if !cc.is_empty() {
let cc: Vec<String> = if input.has_bors_commit {
cc.iter().map(|s| format!("`{s}`")).collect()
Copy link
Member

@RalfJung RalfJung Nov 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

On Zulip there was a proposal to also mark the PR as draft, to prevent it from being merge.

Ideally we'd want a scheme where the pings definitely happen when the PR gets approved or lands, but I am not sure if that is possible.

EDIT: Ah I saw that was already discussed and doesn't seem possible. Bummer.

} else {
cc.to_owned()
};
write!(result, "\n\ncc {}", cc.join(", ")).unwrap();
}
state.data.paths.push(to_mention.to_string());
Expand Down