From cb766affe3fa3579fde278039ad5ab551f8c3b63 Mon Sep 17 00:00:00 2001 From: Urgau Date: Wed, 30 Oct 2024 21:20:17 +0100 Subject: [PATCH 1/2] Also react rendered link handler on synchronize (new commits) --- src/handlers/rendered_link.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/handlers/rendered_link.rs b/src/handlers/rendered_link.rs index 037f0607..1e15fef3 100644 --- a/src/handlers/rendered_link.rs +++ b/src/handlers/rendered_link.rs @@ -34,6 +34,7 @@ async fn add_rendered_link(ctx: &Context, e: &IssuesEvent, prefix: &str) -> anyh if e.action == IssuesAction::Opened || e.action == IssuesAction::Closed || e.action == IssuesAction::Reopened + || e.action == IssuesAction::Synchronize { let files = e.issue.files(&ctx.github).await?; From 18de61aff6416e3db9e6d455110dae6aa3c8c250 Mon Sep 17 00:00:00 2001 From: Urgau Date: Wed, 30 Oct 2024 21:40:00 +0100 Subject: [PATCH 2/2] Remove link if no file and optimize rendered link api calls --- src/handlers/rendered_link.rs | 117 ++++++++++++++++++++-------------- 1 file changed, 68 insertions(+), 49 deletions(-) diff --git a/src/handlers/rendered_link.rs b/src/handlers/rendered_link.rs index 1e15fef3..ec79f57f 100644 --- a/src/handlers/rendered_link.rs +++ b/src/handlers/rendered_link.rs @@ -1,3 +1,5 @@ +use std::borrow::Cow; + use anyhow::bail; use crate::{ @@ -38,62 +40,79 @@ async fn add_rendered_link(ctx: &Context, e: &IssuesEvent, prefix: &str) -> anyh { let files = e.issue.files(&ctx.github).await?; - if let Some(file) = files.iter().find(|f| f.filename.starts_with(prefix)) { - let head = e.issue.head.as_ref().unwrap(); - let base = e.issue.base.as_ref().unwrap(); + let rendered_link = files + .iter() + .find(|f| f.filename.starts_with(prefix)) + .map(|file| { + let head = e.issue.head.as_ref().unwrap(); + let base = e.issue.base.as_ref().unwrap(); - // This URL should be stable while the PR is open, even if the - // user pushes new commits. - // - // It will go away if the user deletes their branch, or if - // they reset it (such as if they created a PR from master). - // That should usually only happen after the PR is closed - // a which point we switch to a SHA-based url. - // - // If the PR is merged we use a URL that points to the actual - // repository, as to be resilient to branch deletion, as well - // be in sync with current "master" branch. - // - // For a PR "octocat:master" <- "Bob:patch-1", we generate, - // - if merged: `https://github.com/octocat/REPO/blob/master/FILEPATH` - // - if open: `https://github.com/Bob/REPO/blob/patch-1/FILEPATH` - // - if closed: `https://github.com/octocat/REPO/blob/SHA/FILEPATH` - let rendered_link = format!( - "[Rendered](https://github.com/{}/blob/{}/{})", - if e.issue.merged || e.action == IssuesAction::Closed { - &e.repository.full_name - } else { - &head.repo.full_name - }, - if e.issue.merged { - &base.git_ref - } else if e.action == IssuesAction::Closed { - &head.sha - } else { - &head.git_ref - }, - file.filename - ); + // This URL should be stable while the PR is open, even if the + // user pushes new commits. + // + // It will go away if the user deletes their branch, or if + // they reset it (such as if they created a PR from master). + // That should usually only happen after the PR is closed + // a which point we switch to a SHA-based url. + // + // If the PR is merged we use a URL that points to the actual + // repository, as to be resilient to branch deletion, as well + // be in sync with current "master" branch. + // + // For a PR "octocat:master" <- "Bob:patch-1", we generate, + // - if merged: `https://github.com/octocat/REPO/blob/master/FILEPATH` + // - if open: `https://github.com/Bob/REPO/blob/patch-1/FILEPATH` + // - if closed: `https://github.com/octocat/REPO/blob/SHA/FILEPATH` + format!( + "[Rendered](https://github.com/{}/blob/{}/{})", + if e.issue.merged || e.action == IssuesAction::Closed { + &e.repository.full_name + } else { + &head.repo.full_name + }, + if e.issue.merged { + &base.git_ref + } else if e.action == IssuesAction::Closed { + &head.sha + } else { + &head.git_ref + }, + file.filename + ) + }); - let new_body = if !e.issue.body.contains("[Rendered]") { + let new_body: Cow<'_, str> = if !e.issue.body.contains("[Rendered]") { + if let Some(rendered_link) = rendered_link { // add rendered link to the end of the body - format!("{}\n\n{rendered_link}", e.issue.body) - } else if let Some(start_pos) = e.issue.body.find("[Rendered](") { - let Some(end_offset) = &e.issue.body[start_pos..].find(')') else { - bail!("no `)` after `[Rendered]` found") - }; + format!("{}\n\n{rendered_link}", e.issue.body).into() + } else { + // or return the original body since we don't have + // a rendered link to add + e.issue.body.as_str().into() + } + } else if let Some(start_pos) = e.issue.body.find("[Rendered](") { + let Some(end_offset) = &e.issue.body[start_pos..].find(')') else { + bail!("no `)` after `[Rendered]` found") + }; - // replace the current rendered link with the new one - e.issue.body.replace( + // replace the current rendered link with the new one or replace + // it with an empty string if we don't have one + e.issue + .body + .replace( &e.issue.body[start_pos..=(start_pos + end_offset)], - &rendered_link, + rendered_link.as_deref().unwrap_or(""), ) - } else { - bail!( - "found `[Rendered]` but not it's associated link, can't replace it, bailing out" - ) - }; + .into() + } else { + bail!( + "found `[Rendered]` but not it's associated link, can't replace it or remove it, bailing out" + ) + }; + // avoid an expensive GitHub api call by first checking if we actually + // edited the pull request body + if e.issue.body != new_body { e.issue.edit_body(&ctx.github, &new_body).await?; } }