From 9c09acb48e2abe77b6fb6e788240e7092a1330c2 Mon Sep 17 00:00:00 2001 From: Peter Tripp Date: Tue, 10 Nov 2026 15:26:22 -0500 Subject: [PATCH 1/2] Make RfdDocument mutators, RfdAsciidoc::new infallible --- rfd-api/src/context.rs | 14 +- rfd-api/src/endpoints/rfd.rs | 27 +--- rfd-data/src/content/asciidoc.rs | 143 +++++++----------- rfd-data/src/content/markdown.rs | 26 ++-- rfd-data/src/content/mod.rs | 78 ++++------ rfd-github/src/lib.rs | 6 +- rfd-processor/src/content/asciidoc.rs | 6 +- rfd-processor/src/content/mod.rs | 47 +++--- rfd-processor/src/rfd.rs | 31 ++-- rfd-processor/src/updater/ensure_pr_state.rs | 3 +- rfd-processor/src/updater/mod.rs | 18 +-- rfd-processor/src/updater/process_includes.rs | 4 +- .../src/updater/update_discussion_url.rs | 3 +- rfd-processor/src/updater/update_pdfs.rs | 1 - 14 files changed, 147 insertions(+), 260 deletions(-) diff --git a/rfd-api/src/context.rs b/rfd-api/src/context.rs index 58f7f40d..6636e42c 100644 --- a/rfd-api/src/context.rs +++ b/rfd-api/src/context.rs @@ -11,7 +11,7 @@ use octorust::{ }; use partial_struct::partial; use rfd_data::{ - content::{RfdContent, RfdContentError, RfdDocument, RfdTemplate, TemplateError}, + content::{RfdContent, RfdDocument, RfdTemplate, TemplateError}, RfdNumber, }; use rfd_github::{GitHubError, GitHubNewRfdNumber, GitHubRfdRepo}; @@ -87,8 +87,6 @@ pub enum UpdateRfdContentError { GitHub(#[from] GitHubError), #[error("Internal GitHub state does not currently allow for update. This commit appears as the head commit on multiple branches.")] InternalState, - #[error("Unable to parse RFD contents")] - InvalidContent(#[from] RfdContentError), #[error("Failed to construct new RFD template")] InvalidTemplate(#[from] TemplateError), #[error("Unable to perform action. Unable to find the default branch on GitHub.")] @@ -676,14 +674,8 @@ impl RfdContext { .inner_err_into()?; let sha = latest_revision.commit.clone(); - let mut updated_content: RfdContent = latest_revision - .try_into() - .map_err(UpdateRfdContentError::InvalidContent) - .map_err(ResourceError::InternalError)?; - updated_content - .update_body(content) - .map_err(UpdateRfdContentError::InvalidContent) - .map_err(ResourceError::InternalError)?; + let mut updated_content: RfdContent = latest_revision.into(); + updated_content.update_body(content); self.commit_rfd_document( caller, diff --git a/rfd-api/src/endpoints/rfd.rs b/rfd-api/src/endpoints/rfd.rs index 7f034549..ec8c8bae 100644 --- a/rfd-api/src/endpoints/rfd.rs +++ b/rfd-api/src/endpoints/rfd.rs @@ -469,13 +469,7 @@ async fn view_rfd_attr_op( let rfd = ctx.view_rfd(caller, rfd_number, None).await?; let content = match (rfd.content, rfd.format) { (Some(content), Some(ContentFormat::Asciidoc)) => { - RfdContent::Asciidoc(RfdAsciidoc::new(content).map_err(|err| { - tracing::warn!(?err, "Failed to parse RFD content"); - HttpError::for_internal_error(format!( - "Failed to parse RFD content for RFD {}", - number - )) - })?) + RfdContent::Asciidoc(RfdAsciidoc::new(content)) } (Some(content), Some(ContentFormat::Markdown)) => { RfdContent::Markdown(RfdMarkdown::new(content)) @@ -760,15 +754,7 @@ async fn set_rfd_attr_op( // TODO: Get rid of these clones let mut content = match revision.content_format { - ContentFormat::Asciidoc => { - RfdContent::Asciidoc(RfdAsciidoc::new(revision.content).map_err(|err| { - tracing::warn!(?err, "Failed to parse RFD content"); - HttpError::for_internal_error(format!( - "Failed to parse RFD content for RFD {}", - number - )) - })?) - } + ContentFormat::Asciidoc => RfdContent::Asciidoc(RfdAsciidoc::new(revision.content)), ContentFormat::Markdown => RfdContent::Markdown(RfdMarkdown::new(revision.content)), }; @@ -783,14 +769,7 @@ async fn set_rfd_attr_op( })?; content.update_state(&state.to_string()) } - } - .map_err(|err| { - tracing::info!(?err, "Update resulted in malfored RFD"); - HttpError::for_bad_request( - None, - "Update would result in a malformed RFD. Update has not been applied".to_string(), - ) - })?; + }; tracing::info!("Updated attribute in RFD document"); diff --git a/rfd-data/src/content/asciidoc.rs b/rfd-data/src/content/asciidoc.rs index 9230a6c4..8715bc86 100644 --- a/rfd-data/src/content/asciidoc.rs +++ b/rfd-data/src/content/asciidoc.rs @@ -4,23 +4,9 @@ use regex::{regex, Regex}; use std::borrow::Cow; -use thiserror::Error; use super::RfdDocument; -static SUPPORTED_REFERENCES: [&str; 1] = ["authors"]; - -#[derive(Debug, Error)] -pub enum RfdAsciidocError { - #[error("Failed to parse author line")] - AuthorParse, - #[error("Failed to resolve reference for {attribute}")] - ReferenceResolver { - attribute: String, - error: regex::Error, - }, -} - /// The text data of an Asciidoc RFD #[derive(Debug, Clone)] pub struct RfdAsciidoc<'a> { @@ -29,46 +15,36 @@ pub struct RfdAsciidoc<'a> { } impl<'a> RfdAsciidoc<'a> { - pub fn new(content: T) -> Result + pub fn new(content: T) -> Self where T: Into>, { let content = content.into(); - let mut resolved = Self::resolve_references(&content)?; + let mut resolved = Self::resolve_references(&content); Self::apply_author_line_attributes(&mut resolved); - Ok(Self { content, resolved }) + Self { content, resolved } } - fn resolve_references(content: &str) -> Result { - let mut resolved = content.to_string(); - - for attribute in SUPPORTED_REFERENCES { - let pattern = - Regex::new(&format!(r#"(?[^\\]?)(\{{{}\}})"#, attribute)).map_err(|err| { - RfdAsciidocError::ReferenceResolver { - attribute: attribute.to_string(), - error: err, - } - })?; - let captures = pattern.captures(&resolved); - - if let Some(captures) = captures { - let prefix = captures.get(1).map(|c| c.as_str()).unwrap_or_default(); - resolved = pattern - .replace_all( - &resolved, - format!( - "{}{}", - prefix, - Self::attr(attribute, &resolved).unwrap_or_default() - ), - ) - .to_string(); - } + /// Resolve attribute references within the document. Currently only `{authors}` is supported. + fn resolve_references(content: &str) -> String { + let pattern = regex!(r"(?[^\\]?)(\{authors\})"); + + if let Some(captures) = pattern.captures(content) { + let prefix = captures.get(1).map(|c| c.as_str()).unwrap_or_default(); + pattern + .replace_all( + content, + format!( + "{}{}", + prefix, + Self::attr("authors", content).unwrap_or_default() + ), + ) + .to_string() + } else { + content.to_string() } - - Ok(resolved) } fn apply_author_line_attributes(content: &mut String) { @@ -186,8 +162,6 @@ impl<'a> AsciidocInclude<'a> { } impl<'a> RfdDocument for RfdAsciidoc<'a> { - type Error = RfdAsciidocError; - fn get_title(&self) -> Option<&str> { let fallback_title_pattern = regex!(r"(?m)^= (.*)$"); @@ -204,7 +178,7 @@ impl<'a> RfdDocument for RfdAsciidoc<'a> { Self::attr("state", &self.resolved) } - fn update_state(&mut self, value: &str) -> Result<&mut Self, Self::Error> { + fn update_state(&mut self, value: &str) -> &mut Self { self.set_raw(&Self::set_attr(&self.content, "state", value.trim())) } @@ -217,7 +191,7 @@ impl<'a> RfdDocument for RfdAsciidoc<'a> { } } - fn update_discussion(&mut self, value: &str) -> Result<&mut Self, Self::Error> { + fn update_discussion(&mut self, value: &str) -> &mut Self { self.set_raw(&Self::set_attr(&self.content, "discussion", value.trim())) } @@ -229,7 +203,7 @@ impl<'a> RfdDocument for RfdAsciidoc<'a> { Self::attr("labels", &self.resolved) } - fn update_labels(&mut self, value: &str) -> Result<&mut Self, Self::Error> { + fn update_labels(&mut self, value: &str) -> &mut Self { self.set_raw(&Self::set_attr(&self.content, "labels", value.trim())) } @@ -241,7 +215,7 @@ impl<'a> RfdDocument for RfdAsciidoc<'a> { RfdAsciidoc::body(&self.content) } - fn update_body(&mut self, value: &str) -> Result<&mut Self, Self::Error> { + fn update_body(&mut self, value: &str) -> &mut Self { self.set_raw(&format!( "{}\n\n{}{}\n{}", self.header().unwrap_or_default(), @@ -255,12 +229,12 @@ impl<'a> RfdDocument for RfdAsciidoc<'a> { &self.content } - fn set_raw(&mut self, content: &str) -> Result<&mut Self, Self::Error> { - let mut resolved = Self::resolve_references(content)?; + fn set_raw(&mut self, content: &str) -> &mut Self { + let mut resolved = Self::resolve_references(content); Self::apply_author_line_attributes(&mut resolved); self.resolved = resolved; self.content = Cow::Owned(content.to_string()); - Ok(self) + self } } @@ -552,7 +526,7 @@ mod tests { {authors} dsfsdf sdf"#; - let rfd = RfdAsciidoc::new(content).unwrap(); + let rfd = RfdAsciidoc::new(content); assert!(rfd.content != rfd.resolved); assert!(!rfd.resolved.contains("{authors}")); } @@ -567,7 +541,7 @@ things , firstname dsfsdf sdf authors: nope"#; - let rfd = RfdAsciidoc::new(content).unwrap(); + let rfd = RfdAsciidoc::new(content); let authors = rfd.get_authors().unwrap(); let expected = r#"things ; firstname "#.to_string(); assert_eq!(expected, authors); @@ -580,7 +554,7 @@ authors: nope"#; {authors} dsfsdf sdf"#; - let rfd = RfdAsciidoc::new(content).unwrap(); + let rfd = RfdAsciidoc::new(content); let authors = rfd.get_authors().unwrap(); let expected = r#"firstname "#.to_string(); assert_eq!(expected, authors); @@ -600,7 +574,7 @@ sdf"#; dsfsdfdsfsdfdsfsdfdsfsdfdsfsdfdsfsdfdsfsdfdsfsdfdsfsdfdsfsdfdsfsdf "#; - let rfd = RfdAsciidoc::new(content).unwrap(); + let rfd = RfdAsciidoc::new(content); let authors = rfd.get_authors().unwrap(); let expected = r#"firstname "#.to_string(); assert_eq!(expected, authors); @@ -618,7 +592,7 @@ dsfsdfdsfsdfdsfsdfdsfsdfdsfsdfdsfsdfdsfsdfdsfsdfdsfsdfdsfsdfdsfsdf ## External References * https://company.com[link to company] - An external reference "#; - let rfd = RfdAsciidoc::new(content).unwrap(); + let rfd = RfdAsciidoc::new(content); let authors = rfd.get_authors().unwrap(); let expected = r#"Author One ; Author Two "#.to_string(); assert_eq!(expected, authors); @@ -640,7 +614,7 @@ dsfsdfdsfsdfdsfsdfdsfsdfdsfsdfdsfsdfdsfsdfdsfsdfdsfsdfdsfsdfdsfsdf == Introduction "#; - let rfd = RfdAsciidoc::new(content).unwrap(); + let rfd = RfdAsciidoc::new(content); let authors = rfd.get_authors().unwrap(); let expected = r#":authors: Author One ; Author Two "#.to_string(); @@ -658,7 +632,7 @@ Author One , Author Two :authors: Author One "#; - let rfd = RfdAsciidoc::new(content).unwrap(); + let rfd = RfdAsciidoc::new(content); let authors = rfd.get_authors().unwrap(); let expected = r#"Author One ; Author Two "#.to_string(); assert_eq!(expected, authors); @@ -674,7 +648,7 @@ Author One , Author Two dsfsdf sdf :state: nope"#; - let rfd = RfdAsciidoc::new(content).unwrap(); + let rfd = RfdAsciidoc::new(content); let state = rfd.get_state().unwrap(); let expected = "prediscussion".to_string(); assert_eq!(expected, state); @@ -690,7 +664,7 @@ sdf dsfsdf sdf :discussion: nope"#; - let rfd = RfdAsciidoc::new(content).unwrap(); + let rfd = RfdAsciidoc::new(content); let discussion = rfd.get_discussion().unwrap(); let expected = "https://github.com/org/repo/pulls/1".to_string(); assert_eq!(expected, discussion); @@ -704,9 +678,8 @@ sdf dsfsdf sdf :discussion: nope"#; - let mut rfd = RfdAsciidoc::new(content).unwrap(); - rfd.set_raw(&RfdAsciidoc::set_attr(rfd.raw(), "xrefstyle", "short")) - .unwrap(); + let mut rfd = RfdAsciidoc::new(content); + rfd.set_raw(&RfdAsciidoc::set_attr(rfd.raw(), "xrefstyle", "short")); assert_eq!(Some("short"), RfdAsciidoc::attr("xrefstyle", &rfd.content)) } @@ -724,8 +697,8 @@ dsfsdf sdf :discussion: nope"#; - let mut rfd = RfdAsciidoc::new(content).unwrap(); - rfd.update_discussion(link).unwrap(); + let mut rfd = RfdAsciidoc::new(content); + rfd.update_discussion(link); let expected = r#"sdfsdf = sdfgsd discussion: fgsdfg @@ -748,8 +721,8 @@ dsfsdf sdf :discussion: nope"#; - let mut rfd = RfdAsciidoc::new(content).unwrap(); - rfd.update_discussion(link).unwrap(); + let mut rfd = RfdAsciidoc::new(content); + rfd.update_discussion(link); let expected = r#"sdfsdf = sdfgsd discussion: fgsdfg @@ -772,8 +745,8 @@ state: fgsdfg dsfsdf sdf :state: nope"#; - let mut rfd = RfdAsciidoc::new(content).unwrap(); - rfd.update_state(state).unwrap(); + let mut rfd = RfdAsciidoc::new(content); + rfd.update_state(state); let expected = r#"sdfsdf = sdfgsd state: fgsdfg @@ -794,8 +767,8 @@ state: fgsdfg dsfsdf sdf :state: nope"#; - let mut rfd = RfdAsciidoc::new(content).unwrap(); - rfd.update_state(state).unwrap(); + let mut rfd = RfdAsciidoc::new(content); + rfd.update_state(state); let expected = r#"sdfsdf = sdfgsd state: fgsdfg @@ -817,7 +790,7 @@ dsfsdf = RFD 53 Bye sdf :title: nope"#; - let rfd = RfdAsciidoc::new(content).unwrap(); + let rfd = RfdAsciidoc::new(content); let expected = "Identity and Access Management (IAM)".to_string(); assert_eq!(expected, rfd.get_title().unwrap()); } @@ -832,7 +805,7 @@ sdf dsfsdf sdf :title: nope"#; - let rfd = RfdAsciidoc::new(content).unwrap(); + let rfd = RfdAsciidoc::new(content); let expected = "Identity and Access Management (IAM)".to_string(); assert_eq!(expected, rfd.get_title().unwrap()); } @@ -846,7 +819,7 @@ sdf dsfsdf sdf :title: nope"#; - let rfd = RfdAsciidoc::new(content).unwrap(); + let rfd = RfdAsciidoc::new(content); let expected = "Identity and Access Management (IAM)".to_string(); assert_eq!(expected, rfd.get_title().unwrap()); } @@ -869,7 +842,7 @@ sdf {authors} "#; - let rfd = RfdAsciidoc::new(Cow::Borrowed(content)).unwrap(); + let rfd = RfdAsciidoc::new(Cow::Borrowed(content)); let expected = "This should be the title"; assert_eq!(expected, rfd.get_title().unwrap()); @@ -918,7 +891,7 @@ in velit. #[test] fn test_get_asciidoc_labels() { - let rfd = RfdAsciidoc::new(Cow::Borrowed(test_rfd_content())).unwrap(); + let rfd = RfdAsciidoc::new(Cow::Borrowed(test_rfd_content())); let labels = rfd.get_labels().unwrap(); let expected = "label1, label2".to_string(); assert_eq!(expected, labels); @@ -930,14 +903,14 @@ in velit. = RFD 43 Identity and Access Management (IAM) No labels here "#; - let rfd = RfdAsciidoc::new(content).unwrap(); + let rfd = RfdAsciidoc::new(content); assert!(rfd.get_labels().is_none()); } #[test] fn test_update_asciidoc_labels() { - let mut rfd = RfdAsciidoc::new(Cow::Borrowed(test_rfd_content())).unwrap(); - rfd.update_labels("newlabel1, newlabel2").unwrap(); + let mut rfd = RfdAsciidoc::new(Cow::Borrowed(test_rfd_content())); + rfd.update_labels("newlabel1, newlabel2"); let labels = rfd.get_labels().unwrap(); let expected = "newlabel1, newlabel2".to_string(); assert_eq!(expected, labels); @@ -961,8 +934,8 @@ in velit. FirstName LastName This is the new body"#; - let mut rfd = RfdAsciidoc::new(Cow::Borrowed(test_rfd_content())).unwrap(); - rfd.update_body(new_content).unwrap(); + let mut rfd = RfdAsciidoc::new(Cow::Borrowed(test_rfd_content())); + rfd.update_body(new_content); assert_eq!(expected, rfd.raw()); } @@ -986,7 +959,7 @@ include::sub_doc1.adoc[] This is the new body include::sub_doc2.adoc[]"#; - let rfd = RfdAsciidoc::new(Cow::Borrowed(contents)).unwrap(); + let rfd = RfdAsciidoc::new(Cow::Borrowed(contents)); let includes = rfd.includes(); let expected = vec![ diff --git a/rfd-data/src/content/markdown.rs b/rfd-data/src/content/markdown.rs index 2a3ab189..2c9bc99d 100644 --- a/rfd-data/src/content/markdown.rs +++ b/rfd-data/src/content/markdown.rs @@ -67,8 +67,6 @@ impl<'a> RfdMarkdown<'a> { } impl<'a> RfdDocument for RfdMarkdown<'a> { - type Error = (); - fn get_title(&self) -> Option<&str> { let title_pattern = regex!(r"(?m)^[=# ]+(?:RFD ?)?(?:\d+:? )?(.*)$"); let fallback_title_pattern = regex!(r"(?m)^# (.*)$"); @@ -86,18 +84,18 @@ impl<'a> RfdDocument for RfdMarkdown<'a> { self.attr("state") } - fn update_state(&mut self, value: &str) -> Result<&mut Self, Self::Error> { + fn update_state(&mut self, value: &str) -> &mut Self { self.set_attr("state", value); - Ok(self) + self } fn get_discussion(&self) -> Option<&str> { self.attr("discussion") } - fn update_discussion(&mut self, value: &str) -> Result<&mut Self, Self::Error> { + fn update_discussion(&mut self, value: &str) -> &mut Self { self.set_attr("discussion", value); - Ok(self) + self } fn get_authors(&self) -> Option<&str> { @@ -108,9 +106,9 @@ impl<'a> RfdDocument for RfdMarkdown<'a> { self.attr("labels") } - fn update_labels(&mut self, value: &str) -> Result<&mut Self, Self::Error> { + fn update_labels(&mut self, value: &str) -> &mut Self { self.set_attr("labels", value); - Ok(self) + self } fn header(&self) -> Option<&str> { @@ -124,7 +122,7 @@ impl<'a> RfdDocument for RfdMarkdown<'a> { self.title_pattern().splitn(&self.content, 2).nth(1) } - fn update_body(&mut self, value: &str) -> Result<&mut Self, Self::Error> { + fn update_body(&mut self, value: &str) -> &mut Self { self.content = Cow::Owned( self.title_pattern() .splitn(&self.content, 2) @@ -133,7 +131,7 @@ impl<'a> RfdDocument for RfdMarkdown<'a> { .to_string() + value, ); - Ok(self) + self } /// Get a reference to the internal unparsed contents @@ -141,9 +139,9 @@ impl<'a> RfdDocument for RfdMarkdown<'a> { &self.content } - fn set_raw(&mut self, content: &str) -> Result<&mut Self, Self::Error> { + fn set_raw(&mut self, content: &str) -> &mut Self { self.content = Cow::Owned(content.to_string()); - Ok(self) + self } } @@ -238,7 +236,7 @@ dsfsdf sdf authors: nope"#; let mut rfd = RfdMarkdown::new(content); - rfd.update_discussion(link).unwrap(); + rfd.update_discussion(link); let expected = r#"sdfsdf sdfsdf @@ -262,7 +260,7 @@ dsfsdf sdf authors: nope"#; let mut rfd = RfdMarkdown::new(content); - rfd.update_state(state).unwrap(); + rfd.update_state(state); let expected = r#"sdfsdf sdfsdf diff --git a/rfd-data/src/content/mod.rs b/rfd-data/src/content/mod.rs index bebbe641..4d1e347a 100644 --- a/rfd-data/src/content/mod.rs +++ b/rfd-data/src/content/mod.rs @@ -7,22 +7,12 @@ mod markdown; mod template; pub use asciidoc::RfdAsciidoc; -pub use asciidoc::RfdAsciidocError; pub use markdown::RfdMarkdown; pub use template::{RenderableRfdTemplate, RfdTemplate, TemplateError}; use rfd_model::{schema_ext::ContentFormat, RfdRevision}; -use thiserror::Error; - -#[derive(Debug, Error)] -pub enum RfdContentError { - #[error("Failed to parse Asciidoc content")] - Asciidoc(#[from] RfdAsciidocError), -} pub trait RfdDocument { - type Error; - /// Extract the title from the internal content fn get_title(&self) -> Option<&str>; @@ -30,13 +20,13 @@ pub trait RfdDocument { fn get_state(&self) -> Option<&str>; // Update the state value stored within the document or add it if it does not exist - fn update_state(&mut self, value: &str) -> Result<&mut Self, Self::Error>; + fn update_state(&mut self, value: &str) -> &mut Self; /// Get the discussion link stored within the document fn get_discussion(&self) -> Option<&str>; // Update the discussion link stored within the document or add it if it does not exist - fn update_discussion(&mut self, value: &str) -> Result<&mut Self, Self::Error>; + fn update_discussion(&mut self, value: &str) -> &mut Self; /// Get the authors line stored within the document. The returned string may contain multiple /// names @@ -46,7 +36,7 @@ pub trait RfdDocument { fn get_labels(&self) -> Option<&str>; // Update the labels stored within the document or add them if they do not exist - fn update_labels(&mut self, value: &str) -> Result<&mut Self, Self::Error>; + fn update_labels(&mut self, value: &str) -> &mut Self; // Get a reference to the contents of the RFD header fn header(&self) -> Option<&str>; @@ -55,13 +45,13 @@ pub trait RfdDocument { fn body(&self) -> Option<&str>; /// Get a reference to the contents of the RFD body - fn update_body(&mut self, value: &str) -> Result<&mut Self, Self::Error>; + fn update_body(&mut self, value: &str) -> &mut Self; /// Get a reference to the internal unparsed contents fn raw(&self) -> &str; /// Set the internal unparsed contents. This may incur a reparsing of the contents - fn set_raw(&mut self, content: &str) -> Result<&mut Self, Self::Error>; + fn set_raw(&mut self, content: &str) -> &mut Self; } #[derive(Debug, Clone)] @@ -80,8 +70,6 @@ impl<'a> RfdContent<'a> { } impl<'a> RfdDocument for RfdContent<'a> { - type Error = RfdContentError; - fn get_title(&self) -> Option<&str> { match self { Self::Asciidoc(inner) => inner.get_title(), @@ -96,17 +84,16 @@ impl<'a> RfdDocument for RfdContent<'a> { } } - fn update_state(&mut self, value: &str) -> Result<&mut Self, Self::Error> { + fn update_state(&mut self, value: &str) -> &mut Self { match self { Self::Asciidoc(inner) => { - inner.update_state(value)?; + inner.update_state(value); } Self::Markdown(inner) => { - // Markdown returns an empty error so we can ignore it - let _ = inner.update_state(value); + inner.update_state(value); } }; - Ok(self) + self } fn get_discussion(&self) -> Option<&str> { @@ -116,17 +103,16 @@ impl<'a> RfdDocument for RfdContent<'a> { } } - fn update_discussion(&mut self, value: &str) -> Result<&mut Self, Self::Error> { + fn update_discussion(&mut self, value: &str) -> &mut Self { match self { Self::Asciidoc(inner) => { - inner.update_discussion(value)?; + inner.update_discussion(value); } Self::Markdown(inner) => { - // Markdown returns an empty error so we can ignore it - let _ = inner.update_discussion(value); + inner.update_discussion(value); } }; - Ok(self) + self } fn get_authors(&self) -> Option<&str> { @@ -143,17 +129,16 @@ impl<'a> RfdDocument for RfdContent<'a> { } } - fn update_labels(&mut self, value: &str) -> Result<&mut Self, Self::Error> { + fn update_labels(&mut self, value: &str) -> &mut Self { match self { Self::Asciidoc(inner) => { - inner.update_labels(value)?; + inner.update_labels(value); } Self::Markdown(inner) => { - // Markdown returns an empty error so we can ignore it - let _ = inner.update_labels(value); + inner.update_labels(value); } }; - Ok(self) + self } fn header(&self) -> Option<&str> { @@ -170,17 +155,16 @@ impl<'a> RfdDocument for RfdContent<'a> { } } - fn update_body(&mut self, value: &str) -> Result<&mut Self, Self::Error> { + fn update_body(&mut self, value: &str) -> &mut Self { match self { Self::Asciidoc(inner) => { - inner.update_body(value)?; + inner.update_body(value); } Self::Markdown(inner) => { - // Markdown returns an empty error so we can ignore it - let _ = inner.update_body(value); + inner.update_body(value); } }; - Ok(self) + self } fn raw(&self) -> &str { @@ -190,26 +174,24 @@ impl<'a> RfdDocument for RfdContent<'a> { } } - fn set_raw(&mut self, content: &str) -> Result<&mut Self, Self::Error> { + fn set_raw(&mut self, content: &str) -> &mut Self { match self { Self::Asciidoc(inner) => { - inner.set_raw(content)?; + inner.set_raw(content); } Self::Markdown(inner) => { - // Markdown returns an empty error so we can ignore it - let _ = inner.set_raw(content); + inner.set_raw(content); } }; - Ok(self) + self } } -impl<'a> TryFrom for RfdContent<'a> { - type Error = RfdContentError; - fn try_from(value: RfdRevision) -> Result { - Ok(match value.content_format { - ContentFormat::Asciidoc => RfdContent::Asciidoc(RfdAsciidoc::new(value.content)?), +impl<'a> From for RfdContent<'a> { + fn from(value: RfdRevision) -> Self { + match value.content_format { + ContentFormat::Asciidoc => RfdContent::Asciidoc(RfdAsciidoc::new(value.content)), ContentFormat::Markdown => RfdContent::Markdown(RfdMarkdown::new(value.content)), - }) + } } } diff --git a/rfd-github/src/lib.rs b/rfd-github/src/lib.rs index c5f6dc1b..ac766d31 100644 --- a/rfd-github/src/lib.rs +++ b/rfd-github/src/lib.rs @@ -23,7 +23,7 @@ use octorust::{ }; use regex::regex; use rfd_data::{ - content::{RfdAsciidoc, RfdAsciidocError, RfdContent}, + content::{RfdAsciidoc, RfdContent}, RfdNumber, }; use rfd_model::{CommitSha, FileSha}; @@ -44,8 +44,6 @@ pub enum GitHubError { FailedToDecodeData(#[from] DecodeError), #[error("Could not find a committer for a commit")] FailedToFindCommitter, - #[error("Failed to parse RFD contents")] - InvalidContent(#[from] RfdAsciidocError), #[error("Failed to parse decoded contents")] InvalidData(#[from] Utf8Error), #[error("Failed to parse date")] @@ -463,7 +461,7 @@ impl GitHubRfdLocation { .fetch_content(client, &readme_path, &self.commit) .await?; - let content = RfdContent::Asciidoc(RfdAsciidoc::new(Cow::Owned(parsed))?); + let content = RfdContent::Asciidoc(RfdAsciidoc::new(Cow::Owned(parsed))); // The html_url for the README.* file will look something like: // https://github.com///blob//rfd//README.* diff --git a/rfd-processor/src/content/asciidoc.rs b/rfd-processor/src/content/asciidoc.rs index d2225c67..d6d36099 100644 --- a/rfd-processor/src/content/asciidoc.rs +++ b/rfd-processor/src/content/asciidoc.rs @@ -3,7 +3,7 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. use async_trait::async_trait; -use rfd_data::content::{RfdAsciidoc, RfdContentError, RfdDocument}; +use rfd_data::content::{RfdAsciidoc, RfdDocument}; use std::{path::PathBuf, process::Command}; use crate::util::write_file; @@ -15,9 +15,7 @@ pub(crate) async fn render_pdf_from_dir( ) -> Result { let content_dir = content_dir.canonicalize()?; let contents = std::fs::read_to_string(content_dir.join("contents.adoc"))?; - let content = RfdAsciidoc::new(contents) - .map_err(RfdContentError::Asciidoc) - .map_err(RenderableRfdError::InvalidRfdContent)?; + let content = RfdAsciidoc::new(contents); render_pdf(&content, content_dir).await } diff --git a/rfd-processor/src/content/mod.rs b/rfd-processor/src/content/mod.rs index b4b7448e..4ed05e41 100644 --- a/rfd-processor/src/content/mod.rs +++ b/rfd-processor/src/content/mod.rs @@ -8,7 +8,7 @@ use async_trait::async_trait; use base64::DecodeError; use octorust::Client; use rfd_data::{ - content::{RfdAsciidoc, RfdContent, RfdContentError, RfdDocument, RfdMarkdown}, + content::{RfdAsciidoc, RfdContent, RfdDocument, RfdMarkdown}, RfdNumber, }; use rfd_github::{GitHubError, GitHubRfdLocation}; @@ -36,8 +36,6 @@ pub enum RenderableRfdError { GitHub(#[from] GitHubError), #[error("Failed to convert content string {0}")] InvalidContent(#[from] Utf8Error), - #[error("Failed to parse Rfd contents")] - InvalidRfdContent(#[from] RfdContentError), #[error("General io failure {0}")] Io(#[from] io::Error), #[error("File io failure {0}")] @@ -70,17 +68,14 @@ impl<'a> RenderableRfd<'a> { } /// Construct a new RfdContent wrapper that contains Asciidoc content - #[allow(clippy::result_large_err)] - pub fn new_asciidoc(content: T) -> Result + pub fn new_asciidoc(content: T) -> Self where T: Into>, { - Ok(Self { - content: RfdContent::Asciidoc( - RfdAsciidoc::new(content.into()).map_err(RfdContentError::Asciidoc)?, - ), + Self { + content: RfdContent::Asciidoc(RfdAsciidoc::new(content.into())), render_id: Uuid::new_v4(), - }) + } } /// Construct a new RfdContent wrapper that contains Markdown content @@ -200,8 +195,6 @@ impl<'a> RenderableRfd<'a> { } impl<'a> RfdDocument for RenderableRfd<'a> { - type Error = RenderableRfdError; - fn get_title(&self) -> Option<&str> { RfdDocument::get_title(&self.content) } @@ -210,18 +203,18 @@ impl<'a> RfdDocument for RenderableRfd<'a> { RfdDocument::get_state(&self.content) } - fn update_state(&mut self, value: &str) -> Result<&mut Self, Self::Error> { - RfdDocument::update_state(&mut self.content, value)?; - Ok(self) + fn update_state(&mut self, value: &str) -> &mut Self { + RfdDocument::update_state(&mut self.content, value); + self } fn get_discussion(&self) -> Option<&str> { RfdDocument::get_discussion(&self.content) } - fn update_discussion(&mut self, value: &str) -> Result<&mut Self, Self::Error> { - RfdDocument::update_discussion(&mut self.content, value)?; - Ok(self) + fn update_discussion(&mut self, value: &str) -> &mut Self { + RfdDocument::update_discussion(&mut self.content, value); + self } fn get_authors(&self) -> Option<&str> { @@ -232,9 +225,9 @@ impl<'a> RfdDocument for RenderableRfd<'a> { RfdDocument::get_labels(&self.content) } - fn update_labels(&mut self, value: &str) -> Result<&mut Self, Self::Error> { - RfdDocument::update_labels(&mut self.content, value)?; - Ok(self) + fn update_labels(&mut self, value: &str) -> &mut Self { + RfdDocument::update_labels(&mut self.content, value); + self } fn header(&self) -> Option<&str> { @@ -245,18 +238,18 @@ impl<'a> RfdDocument for RenderableRfd<'a> { RfdDocument::body(&self.content) } - fn update_body(&mut self, value: &str) -> Result<&mut Self, Self::Error> { - RfdDocument::update_body(&mut self.content, value)?; - Ok(self) + fn update_body(&mut self, value: &str) -> &mut Self { + RfdDocument::update_body(&mut self.content, value); + self } fn raw(&self) -> &str { RfdDocument::raw(&self.content) } - fn set_raw(&mut self, content: &str) -> Result<&mut Self, Self::Error> { - self.content.set_raw(content)?; - Ok(self) + fn set_raw(&mut self, content: &str) -> &mut Self { + self.content.set_raw(content); + self } } diff --git a/rfd-processor/src/rfd.rs b/rfd-processor/src/rfd.rs index 73f35683..347cf721 100644 --- a/rfd-processor/src/rfd.rs +++ b/rfd-processor/src/rfd.rs @@ -19,7 +19,7 @@ use rfd_model::{ use thiserror::Error; use v_model::storage::{ListPagination, StoreError}; -use crate::content::{RenderableRfd, RenderableRfdError}; +use crate::content::RenderableRfd; #[derive(Debug)] pub struct PersistedRfd { @@ -117,12 +117,11 @@ impl PersistedRfd { Ok(()) } - #[allow(clippy::result_large_err)] - pub fn content(&self) -> Result, RenderableRfdError> { - Ok(match self.revision.content_format { - ContentFormat::Asciidoc => RenderableRfd::new_asciidoc(&self.revision.content)?, + pub fn content(&self) -> RenderableRfd<'_> { + match self.revision.content_format { + ContentFormat::Asciidoc => RenderableRfd::new_asciidoc(&self.revision.content), ContentFormat::Markdown => RenderableRfd::new_markdown(&self.revision.content), - }) + } } pub fn set_content(&mut self, format: ContentFormat, content: &str) { @@ -132,19 +131,16 @@ impl PersistedRfd { *self.needs_update.lock().unwrap() = true; } - #[allow(clippy::result_large_err)] - pub fn update_discussion(&mut self, new_discussion_url: impl ToString) -> Result<(), RfdError> { + pub fn update_discussion(&mut self, new_discussion_url: impl ToString) { let new_discussion_url = new_discussion_url.to_string(); - let mut content = self.content()?; - content.update_discussion(&new_discussion_url)?; + let mut content = self.content(); + content.update_discussion(&new_discussion_url); self.revision.content = content.into_inner_content(); self.revision.discussion = Some(new_discussion_url); *self.needs_update.lock().unwrap() = true; - - Ok(()) } pub fn is_state(&self, state: &str) -> bool { @@ -155,19 +151,16 @@ impl PersistedRfd { .unwrap_or(false) } - #[allow(clippy::result_large_err)] - pub fn update_state(&mut self, new_state: impl ToString) -> Result<(), RfdError> { + pub fn update_state(&mut self, new_state: impl ToString) { let new_state = new_state.to_string(); - let mut content = self.content()?; - content.update_state(&new_state)?; + let mut content = self.content(); + content.update_state(&new_state); self.revision.content = content.into_inner_content(); self.revision.state = Some(new_state); *self.needs_update.lock().unwrap() = true; - - Ok(()) } pub fn get_pdf_filename(&self) -> String { @@ -196,8 +189,6 @@ impl PersistedRfd { #[derive(Debug, Error)] pub enum RfdError { - #[error(transparent)] - Asciidoc(#[from] RenderableRfdError), #[error(transparent)] Storage(#[from] StoreError), } diff --git a/rfd-processor/src/updater/ensure_pr_state.rs b/rfd-processor/src/updater/ensure_pr_state.rs index 151ebba9..edac0d7f 100644 --- a/rfd-processor/src/updater/ensure_pr_state.rs +++ b/rfd-processor/src/updater/ensure_pr_state.rs @@ -58,8 +58,7 @@ impl RfdUpdateAction for EnsureRfdWithPullRequestIsInValidState { && !new.is_state("ideation") && !new.is_state("abandoned") { - new.update_state("discussion") - .map_err(|err| RfdUpdateActionErr::Stop(Box::new(err)))?; + new.update_state("discussion"); requires_source_commit = true; } else { tracing::debug!("RFD is in a valid state and does not need to be updated"); diff --git a/rfd-processor/src/updater/mod.rs b/rfd-processor/src/updater/mod.rs index 9f84bc44..2e3254df 100644 --- a/rfd-processor/src/updater/mod.rs +++ b/rfd-processor/src/updater/mod.rs @@ -16,7 +16,6 @@ use tracing::instrument; use v_model::storage::StoreError; use crate::{ - content::RenderableRfdError, context::Context, rfd::{FetchRemoteRfdError, PersistedRfd, RemoteRfd, RemoteRfdError, RfdError}, }; @@ -59,8 +58,6 @@ pub enum RfdUpdaterError { RfdUpdate(RfdError), #[error("Newly persisted RFD has an internal id that does match the expected existing id")] RfdIdMismatch((TypedUuid, TypedUuid)), - #[error("Failed to retrieve the content of the previous revision (to detect if this is a major change)")] - ContentOfExistingRfd(#[source] RenderableRfdError), } trait Validate { @@ -180,13 +177,7 @@ impl<'a> RfdUpdater<'a> { if existing.revision.commit == remote.commit { existing.revision.major_change } else { - is_major_change( - &existing - .content() - .map_err(RfdUpdaterError::ContentOfExistingRfd)? - .content, - &remote.readme.content, - ) + is_major_change(&existing.content().content, &remote.readme.content) } } else { tracing::info!(number = ?update.number, "No previous revisions found for RFD"); @@ -283,10 +274,7 @@ impl<'a> RfdUpdater<'a> { .location .upsert( &new.number, - new.content() - .map_err(|err| RfdUpdaterError::RfdUpdate(RfdError::Asciidoc(err)))? - .raw() - .as_bytes(), + new.content().raw().as_bytes(), "RFD processor update", ) .await @@ -407,7 +395,7 @@ mod tests { const ADOC: &str = ":state: discussion\n:discussion: https://github.com/oxidecomputer/rfd/pull/1\n\n= RFD 1 Test\nAuthor \n\nSome text with trailing whitespace. \nAnother line.\n"; fn adoc(content: &str) -> RfdContent<'static> { - RfdContent::Asciidoc(RfdAsciidoc::new(content.to_string()).unwrap()) + RfdContent::Asciidoc(RfdAsciidoc::new(content.to_string())) } #[test] diff --git a/rfd-processor/src/updater/process_includes.rs b/rfd-processor/src/updater/process_includes.rs index 3523a90f..f2b1f0bd 100644 --- a/rfd-processor/src/updater/process_includes.rs +++ b/rfd-processor/src/updater/process_includes.rs @@ -30,9 +30,7 @@ impl RfdUpdateAction for ProcessIncludes { tracing::info!("Processing includes"); let RfdUpdateActionContext { ctx, update, .. } = ctx; - let content = new - .content() - .map_err(|err| RfdUpdateActionErr::Continue(Box::new(err)))?; + let content = new.content(); let (format, new_content) = match &content.content { RfdContent::Asciidoc(adoc) => { let mut raw = adoc.raw().to_string(); diff --git a/rfd-processor/src/updater/update_discussion_url.rs b/rfd-processor/src/updater/update_discussion_url.rs index bdabc318..0b5f6dcf 100644 --- a/rfd-processor/src/updater/update_discussion_url.rs +++ b/rfd-processor/src/updater/update_discussion_url.rs @@ -58,8 +58,7 @@ impl RfdUpdateAction for UpdateDiscussionUrl { "Stored discussion link does not match the pull request found" ); - new.update_discussion(&pull_request.html_url) - .map_err(|err| RfdUpdateActionErr::Continue(Box::new(err)))?; + new.update_discussion(&pull_request.html_url); tracing::info!("Updated RFD file in GitHub with discussion link change"); diff --git a/rfd-processor/src/updater/update_pdfs.rs b/rfd-processor/src/updater/update_pdfs.rs index 365dcac0..fd0b8998 100644 --- a/rfd-processor/src/updater/update_pdfs.rs +++ b/rfd-processor/src/updater/update_pdfs.rs @@ -38,7 +38,6 @@ impl UpdatePdfs { // Generate the PDFs for the RFD let pdf = match new .content() - .map_err(RfdOutputError::ContentFailure)? .to_pdf(&ctx.github.client, &update.number, &update.location) .await { From 3e04624161d18d2ee9aa4623c50c509523200485 Mon Sep 17 00:00:00 2001 From: Peter Tripp Date: Tue, 10 Nov 2026 15:40:17 -0500 Subject: [PATCH 2/2] Fewer runtime regexes --- rfd-data/src/content/asciidoc.rs | 15 +++++++++++++-- rfd-data/src/content/markdown.rs | 15 +++++++++++++-- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/rfd-data/src/content/asciidoc.rs b/rfd-data/src/content/asciidoc.rs index 8715bc86..1f5dcbe1 100644 --- a/rfd-data/src/content/asciidoc.rs +++ b/rfd-data/src/content/asciidoc.rs @@ -90,8 +90,19 @@ impl<'a> RfdAsciidoc<'a> { } } - fn attr_pattern(attr: &str) -> Regex { - Regex::new(&format!(r"(?m)^:{}:(.*)$\n", attr)).unwrap() + /// Patterns for the attributes the RFD tooling reads and writes are compiled once. + /// Any other attribute name falls back to compiling a pattern on demand. + fn attr_pattern(attr: &str) -> Cow<'static, Regex> { + match attr { + "state" => Cow::Borrowed(regex!(r"(?m)^:state:(.*)$\n")), + "discussion" => Cow::Borrowed(regex!(r"(?m)^:discussion:(.*)$\n")), + "authors" => Cow::Borrowed(regex!(r"(?m)^:authors:(.*)$\n")), + "labels" => Cow::Borrowed(regex!(r"(?m)^:labels:(.*)$\n")), + _ => Cow::Owned( + Regex::new(&format!(r"(?m)^:{}:(.*)$\n", regex::escape(attr))) + .expect("escaped attribute name should always form a valid pattern"), + ), + } } fn title_line(content: &str) -> Option<&str> { diff --git a/rfd-data/src/content/markdown.rs b/rfd-data/src/content/markdown.rs index 2c9bc99d..95217141 100644 --- a/rfd-data/src/content/markdown.rs +++ b/rfd-data/src/content/markdown.rs @@ -57,8 +57,19 @@ impl<'a> RfdMarkdown<'a> { } } - fn attr_pattern(&self, attr: &str) -> Regex { - Regex::new(&format!(r"(?m)^{}:(.*)$\n", attr)).unwrap() + /// Patterns for the attributes the RFD tooling reads and writes are compiled once. Any other + /// attribute name falls back to compiling a pattern on demand. + fn attr_pattern(&self, attr: &str) -> Cow<'static, Regex> { + match attr { + "state" => Cow::Borrowed(regex!(r"(?m)^state:(.*)$\n")), + "discussion" => Cow::Borrowed(regex!(r"(?m)^discussion:(.*)$\n")), + "authors" => Cow::Borrowed(regex!(r"(?m)^authors:(.*)$\n")), + "labels" => Cow::Borrowed(regex!(r"(?m)^labels:(.*)$\n")), + _ => Cow::Owned( + Regex::new(&format!(r"(?m)^{}:(.*)$\n", regex::escape(attr))) + .expect("escaped attribute name produces a valid pattern"), + ), + } } fn title_pattern(&self) -> &'static Regex {