Skip to content

Improve [rendered-link] guess of the relevant file to point to #2118

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.

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 6 additions & 1 deletion src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -496,7 +496,11 @@ pub(crate) struct BotPullRequests {}
#[serde(rename_all = "kebab-case")]
#[serde(deny_unknown_fields)]
pub(crate) struct RenderedLinkConfig {
/// List of paths to watch for modifications
pub(crate) trigger_files: Vec<String>,
/// List of paths to exclude from watching for modifications
#[serde(default)]
pub(crate) exclude_files: Vec<String>,
}

#[derive(PartialEq, Eq, Debug, serde::Deserialize)]
Expand Down Expand Up @@ -720,7 +724,8 @@ mod tests {
merge_conflicts: None,
bot_pull_requests: None,
rendered_link: Some(RenderedLinkConfig {
trigger_files: vec!["posts/".to_string()]
trigger_files: vec!["posts/".to_string()],
exclude_files: vec![],
}),
issue_links: Some(IssueLinksConfig {
check_commits: true,
Expand Down
3 changes: 3 additions & 0 deletions src/github.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1100,6 +1100,9 @@ pub struct PullRequestFile {
pub sha: String,
pub filename: String,
pub blob_url: String,
pub additions: u64,
pub deletions: u64,
pub changes: u64,
}

#[derive(Debug, serde::Deserialize)]
Expand Down
14 changes: 11 additions & 3 deletions src/handlers/rendered_link.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,20 @@ async fn add_rendered_link(

let rendered_link = files
.iter()
.find(|f| {
.filter(|f| {
config
.trigger_files
.iter()
.any(|tf| f.filename.starts_with(tf))
&& !config
.exclude_files
.iter()
.any(|tf| f.filename.starts_with(tf))
Copy link
Member

Choose a reason for hiding this comment

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

This won't work, because the filename contains the whole path from root (try gh api repos/rust-lang/rust-forge/pulls/918/files). Maybe we could check only the filename itself here, not the whole path?

Probably it would be also more accurate to rename trigger_files to trigger_paths...

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, it's on purpose. exclude_files is the mirror of trigger_paths which works on the filepath.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I know. It's just that the SUMMARY.md default isn't very useful here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I though the SUMMARY.md was at the top for most repos but that doesn't seems to be the case. Removed it as the default.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the intention to put index .md files into exclude_files? What happens if a patch to the rust-forge repository only changes the file SUMMARY.md and that file is in the exclude list? That might be a corner case.

maybe put exclude_files back in if the filtering returned nothing? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

How does this handler work when the patchset does not find .md files to render in trigger_files?

Copy link
Member Author

@Urgau Urgau Jul 18, 2025

Choose a reason for hiding this comment

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

If there no relevant files, then nothing happens, the Rendered link link is not put in the body.

(technically it's a bit more complex since there could have been a match from a previous link from a previous push, so we need to remove it, but that's already handled below)

})
.and_then(|file| {
// Sort the relavant files by the total number of lines changed, as to
// improve our guess for the relevant file to show the link to.
.max_by_key(|f| f.additions + f.deletions + f.changes)
.map(|file| {
let head = e.issue.head.as_ref()?;
let base = e.issue.base.as_ref()?;

Expand Down Expand Up @@ -84,7 +91,8 @@ async fn add_rendered_link(
},
file.filename
))
});
})
.flatten();

let new_body: Cow<'_, str> = if !e.issue.body.contains("[Rendered]") {
if let Some(rendered_link) = rendered_link {
Expand Down