From 5b8b035e09306dffa6614a5313e9aa9c44acc4e0 Mon Sep 17 00:00:00 2001 From: Lucien Cartier-Tilet Date: Sat, 14 Mar 2026 01:27:32 +0100 Subject: [PATCH] feat: edit body for commit messages --- .github/workflows/action.yml | 4 +- Cargo.lock | 1 + Cargo.toml | 2 +- src/commit/types/body.rs | 185 ++++++++++++++++++++++++++++ src/commit/types/message.rs | 227 +++++++++++++++++++++++------------ src/commit/types/mod.rs | 3 + src/lib.rs | 2 +- src/prompts/mock.rs | 33 ++++- src/prompts/prompter.rs | 37 +++++- src/prompts/workflow.rs | 162 ++++++++++++++++++++++++- tests/cli.rs | 4 +- 11 files changed, 569 insertions(+), 91 deletions(-) create mode 100644 src/commit/types/body.rs diff --git a/.github/workflows/action.yml b/.github/workflows/action.yml index 59a2ef1..873fc5a 100644 --- a/.github/workflows/action.yml +++ b/.github/workflows/action.yml @@ -62,7 +62,7 @@ jobs: - name: Upload Linux artifact uses: actions/upload-artifact@v3 with: - name: jj-cz-linux-x86_64 + name: jj-cz-x86_64-unknown-linux-gnu path: dist-linux/* - name: Build Windows release binary @@ -77,5 +77,5 @@ jobs: - name: Upload Windows artifact uses: actions/upload-artifact@v3 with: - name: jj-cz-windows-x86_64 + name: jj-cz-x86_64-pc-windows-gnu path: dist-windows/* diff --git a/Cargo.lock b/Cargo.lock index 62baedc..367c7a2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1571,6 +1571,7 @@ dependencies = [ "crossterm", "dyn-clone", "fuzzy-matcher", + "tempfile", "unicode-segmentation", "unicode-width", ] diff --git a/Cargo.toml b/Cargo.toml index dfca6b7..261f33e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -25,7 +25,7 @@ async-trait = "0.1.89" etcetera = "0.11.0" clap = { version = "4.5.57", features = ["derive"] } git-conventional = "0.12.9" -inquire = "0.9.2" +inquire = { version = "0.9.2", features = ["editor"] } jj-lib = "0.39.0" lazy-regex = { version = "3.5.1", features = ["lite"] } thiserror = "2.0.18" diff --git a/src/commit/types/body.rs b/src/commit/types/body.rs new file mode 100644 index 0000000..885ab3e --- /dev/null +++ b/src/commit/types/body.rs @@ -0,0 +1,185 @@ +#[derive(Debug, Default, Clone, PartialEq, Eq)] +#[repr(transparent)] +pub struct Body(Option); + +impl From for Body { + fn from(value: T) -> Self { + let value = value.to_string(); + let lines: Vec<&str> = value + .trim_end() + .lines() + .map(|line| line.trim_end()) + .skip_while(|line| line.is_empty()) + .collect(); + match lines.join("\n").as_str() { + "" => Self::default(), + value => Self(Some(value.into())), + } + } +} + +impl Body { + pub fn format(&self) -> String { + match &self.0 { + None => String::new(), + Some(value) if value.trim().is_empty() => String::new(), + Some(body) => format!("\n{body}\n"), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Default produces Body(None) — no body + #[test] + fn default_produces_none() { + assert_eq!(Body::default(), Body(None)); + } + + /// Empty string produces Body(None) + #[test] + fn from_empty_string_produces_none() { + assert_eq!(Body::from(""), Body(None)); + } + + /// Whitespace-only string produces Body(None) + #[test] + fn from_whitespace_only_produces_none() { + assert_eq!(Body::from(" "), Body(None)); + } + + /// Tabs and newlines only produce Body(None) + #[test] + fn from_tab_and_newline_only_produces_none() { + assert_eq!(Body::from("\t\n "), Body(None)); + } + + /// A single newline (typical empty-editor save) produces Body(None) + #[test] + fn from_single_newline_produces_none() { + assert_eq!(Body::from("\n"), Body(None)); + } + + /// Non-empty string produces Body(Some(...)) with content preserved + #[test] + fn from_non_empty_string_produces_some() { + assert_eq!( + Body::from("some body text"), + Body(Some("some body text".to_string())), + ); + } + + /// Leading and internal whitespace is preserved — users may write + /// indented lists, ASCII art, file trees, etc. + #[test] + fn from_preserves_leading_whitespace() { + assert_eq!( + Body::from(" content "), + Body(Some(" content".to_string())), + ); + } + + /// Leading whitespace on individual lines is preserved + #[test] + fn from_preserves_per_line_leading_whitespace() { + let input = "- item one\n - nested item\n - deeply nested"; + assert_eq!(Body::from(input), Body(Some(input.to_string())),); + } + + /// Trailing newline (typical editor output) is stripped + #[test] + fn from_trims_trailing_newline() { + assert_eq!( + Body::from("editor content\n"), + Body(Some("editor content".to_string())), + ); + } + + /// Leading blank lines (e.g. from editor artefacts after JJ: comment + /// stripping) are dropped + #[test] + fn from_drops_leading_blank_lines() { + assert_eq!(Body::from("\n\ncontent"), Body(Some("content".to_string())),); + } + + /// Windows-style CRLF line endings are normalised to LF + #[test] + fn from_normalises_crlf_to_lf() { + assert_eq!( + Body::from("line one\r\nline two"), + Body(Some("line one\nline two".to_string())), + ); + } + + /// Internal newlines are preserved for multi-line bodies + #[test] + fn from_preserves_internal_newlines() { + assert_eq!( + Body::from("line one\nline two"), + Body(Some("line one\nline two".to_string())), + ); + } + + /// Into conversion works via `.into()` + #[test] + fn into_conversion_works() { + let body: Body = "content".into(); + assert_eq!(body, Body(Some("content".to_string()))); + } + + /// Clone produces a value equal to the original + #[test] + fn clone_produces_equal_value() { + let body = Body::from("content"); + assert_eq!(body.clone(), body); + } + + /// Two bodies constructed from the same string are equal + #[test] + fn equality_same_content() { + assert_eq!(Body::from("same"), Body::from("same")); + } + + /// Bodies with different content are not equal + #[test] + fn inequality_different_content() { + assert_ne!(Body::from("first"), Body::from("second")); + } + + /// None body is not equal to a body with content + #[test] + fn inequality_none_vs_some() { + assert_ne!(Body::default(), Body::from("content")); + } + + /// Debug output is available and mentions Body + #[test] + fn debug_output_is_available() { + let body = Body::from("test"); + assert!(format!("{:?}", body).contains("Body")); + } + + /// format() on a None body returns an empty string + #[test] + fn format_none_returns_empty_string() { + assert_eq!(Body::default().format(), ""); + } + + /// format() on a Some body returns "\ncontent\n" + /// (leading \n creates the blank line after the commit header; + /// trailing \n creates the blank line before the footer) + #[test] + fn format_some_returns_newline_wrapped_content() { + let body = Body::from("some body text"); + assert_eq!(body.format(), "\nsome body text\n"); + } + + /// format() preserves internal newlines in multi-line bodies + #[test] + fn format_some_multiline_preserves_content() { + let body = Body::from("line one\nline two"); + assert_eq!(body.format(), "\nline one\nline two\n"); + } +} diff --git a/src/commit/types/message.rs b/src/commit/types/message.rs index 7e61f7c..4d9f52a 100644 --- a/src/commit/types/message.rs +++ b/src/commit/types/message.rs @@ -1,4 +1,4 @@ -use super::{BreakingChange, CommitType, Description, Scope}; +use super::{Body, BreakingChange, CommitType, Description, Scope}; use thiserror::Error; /// Errors that can occur when creating a ConventionalCommit @@ -22,6 +22,7 @@ pub struct ConventionalCommit { scope: Scope, description: Description, breaking_change: BreakingChange, + body: Body, } impl ConventionalCommit { @@ -42,12 +43,14 @@ impl ConventionalCommit { scope: Scope, description: Description, breaking_change: BreakingChange, + body: Body, ) -> Result { let commit = Self { commit_type, scope, description, breaking_change, + body, }; let len = commit.first_line_len(); if len > Self::FIRST_LINE_MAX_LENGTH { @@ -88,6 +91,7 @@ impl ConventionalCommit { &self.scope, &self.description, &self.breaking_change, + &self.body, ) } @@ -101,33 +105,20 @@ impl ConventionalCommit { scope: &Scope, description: &Description, breaking_change: &BreakingChange, + body: &Body, ) -> String { let scope = scope.header_segment(); let breaking_change_header = breaking_change.header_segment(); let breaking_change_footer = breaking_change.as_footer(); format!( r#"{commit_type}{scope}{breaking_change_header}: {description} - +{} {breaking_change_footer}"#, + body.format() ) .trim() .to_string() } - - /// Returns the commit type - pub fn commit_type(&self) -> CommitType { - self.commit_type - } - - /// Returns a reference to the scope - pub fn scope(&self) -> &Scope { - &self.scope - } - - /// Returns a reference to the description - pub fn description(&self) -> &Description { - &self.description - } } impl std::fmt::Display for ConventionalCommit { @@ -157,8 +148,14 @@ mod tests { description: Description, breaking_change: BreakingChange, ) -> ConventionalCommit { - ConventionalCommit::new(commit_type, scope, description, breaking_change) - .expect("test commit should have valid line length") + ConventionalCommit::new( + commit_type, + scope, + description, + breaking_change, + Body::default(), + ) + .expect("test commit should have valid line length") } /// Test that ConventionalCommit::new() creates a valid commit with all fields @@ -170,9 +167,9 @@ mod tests { test_description("add new feature"), BreakingChange::No, ); - assert_eq!(commit.commit_type(), CommitType::Feat); - assert_eq!(commit.scope().as_str(), "cli"); - assert_eq!(commit.description().as_str(), "add new feature"); + assert_eq!(commit.commit_type, CommitType::Feat); + assert_eq!(commit.scope.as_str(), "cli"); + assert_eq!(commit.description.as_str(), "add new feature"); } /// Test that ConventionalCommit::new() works with empty scope @@ -184,9 +181,9 @@ mod tests { test_description("fix critical bug"), BreakingChange::No, ); - assert_eq!(commit.commit_type(), CommitType::Fix); - assert!(commit.scope().is_empty()); - assert_eq!(commit.description().as_str(), "fix critical bug"); + assert_eq!(commit.commit_type, CommitType::Fix); + assert!(commit.scope.is_empty()); + assert_eq!(commit.description.as_str(), "fix critical bug"); } /// Test that format() produces "type(scope): description" when scope is non-empty @@ -405,56 +402,6 @@ mod tests { } } - /// Test commit_type() returns the correct type - #[test] - fn commit_type_accessor_returns_correct_type() { - for commit_type in CommitType::all() { - let commit = test_commit( - *commit_type, - Scope::empty(), - test_description("test"), - BreakingChange::No, - ); - assert_eq!(commit.commit_type(), *commit_type); - } - } - - /// Test scope() returns reference to scope - #[test] - fn scope_accessor_returns_reference() { - let commit = test_commit( - CommitType::Feat, - test_scope("auth"), - test_description("add feature"), - BreakingChange::No, - ); - assert_eq!(commit.scope().as_str(), "auth"); - } - - /// Test scope() returns reference to empty scope - #[test] - fn scope_accessor_returns_empty_scope() { - let commit = test_commit( - CommitType::Feat, - Scope::empty(), - test_description("add feature"), - BreakingChange::No, - ); - assert!(commit.scope().is_empty()); - } - - /// Test description() returns reference to description - #[test] - fn description_accessor_returns_reference() { - let commit = test_commit( - CommitType::Feat, - Scope::empty(), - test_description("add new authentication flow"), - BreakingChange::No, - ); - assert_eq!(commit.description().as_str(), "add new authentication flow"); - } - /// Test Clone trait #[test] fn conventional_commit_is_cloneable() { @@ -689,6 +636,7 @@ mod tests { Scope::parse(&scope_20).unwrap(), Description::parse(&desc_44).unwrap(), BreakingChange::No, + Body::default(), ); assert!(result.is_ok()); let commit = result.unwrap(); @@ -718,6 +666,7 @@ mod tests { Scope::parse(&scope_30).unwrap(), Description::parse(&desc_31).unwrap(), BreakingChange::No, + Body::default(), ); assert!(result.is_err()); assert_eq!( @@ -741,6 +690,7 @@ mod tests { Scope::parse(&scope_30).unwrap(), Description::parse(&desc_40).unwrap(), BreakingChange::No, + Body::default(), ); assert!(result.is_err()); assert_eq!( @@ -760,6 +710,7 @@ mod tests { Scope::empty(), test_description("quick fix"), BreakingChange::No, + Body::default(), ); assert!(result.is_ok()); } @@ -772,6 +723,7 @@ mod tests { test_scope("cli"), test_description("add feature"), BreakingChange::No, + Body::default(), ); assert!(result.is_ok()); } @@ -797,6 +749,7 @@ mod tests { Scope::empty(), test_description("test"), BreakingChange::No, + Body::default(), ); // Just verify it's a Result by using is_ok() assert!(result.is_ok()); @@ -822,7 +775,13 @@ mod tests { None => Scope::empty(), }; let desc = Description::parse(*desc_str).unwrap(); - let commit = ConventionalCommit::new(*commit_type, scope, desc, BreakingChange::No); + let commit = ConventionalCommit::new( + *commit_type, + scope, + desc, + BreakingChange::No, + Body::default(), + ); // new() itself calls git_conventional::Commit::parse internally, so // if this is Ok, SC-002 is satisfied for this case. assert!( @@ -958,6 +917,7 @@ mod tests { Scope::parse(&scope_20).unwrap(), Description::parse(&desc_44).unwrap(), BreakingChange::Yes, + Body::default(), ); assert!(result.is_err()); assert_eq!( @@ -979,6 +939,7 @@ mod tests { Scope::empty(), test_description("quick fix"), long_note.into(), + Body::default(), ); assert!(result.is_ok()); } @@ -993,10 +954,11 @@ mod tests { BreakingChange::No, ); let preview = ConventionalCommit::format_preview( - commit.commit_type(), - commit.scope(), - commit.description(), + commit.commit_type, + &commit.scope, + &commit.description, &BreakingChange::No, + &Body::default(), ); assert_eq!(preview, commit.format()); } @@ -1009,6 +971,7 @@ mod tests { &Scope::empty(), &test_description("drop legacy API"), &"removes legacy endpoint".into(), + &Body::default(), ); assert_eq!( preview, @@ -1024,6 +987,7 @@ mod tests { &test_scope("api"), &test_description("drop Node 6"), &"Node 6 is no longer supported".into(), + &Body::default(), ); assert_eq!( preview, @@ -1115,4 +1079,109 @@ mod tests { ); } } + + // --- Body tests (these will fail until format_preview() is updated to use body) --- + + /// format() includes the body between the header and an empty footer + /// + /// Case: body = Some, footer = "" → "type: desc\n\ncontent" + #[test] + fn format_with_body_no_breaking_change() { + let commit = ConventionalCommit::new( + CommitType::Feat, + Scope::empty(), + test_description("add feature"), + BreakingChange::No, + Body::from("This explains the change."), + ) + .unwrap(); + assert_eq!( + commit.format(), + "feat: add feature\n\nThis explains the change." + ); + } + + /// format() includes the body when a scope is also present + #[test] + fn format_with_body_and_scope() { + let commit = ConventionalCommit::new( + CommitType::Fix, + test_scope("api"), + test_description("handle null response"), + BreakingChange::No, + Body::from("Null responses were previously unhandled."), + ) + .unwrap(); + assert_eq!( + commit.format(), + "fix(api): handle null response\n\nNull responses were previously unhandled." + ); + } + + /// format() preserves internal newlines in a multi-paragraph body + #[test] + fn format_with_multiline_body() { + let commit = ConventionalCommit::new( + CommitType::Docs, + Scope::empty(), + test_description("update README"), + BreakingChange::No, + Body::from("First paragraph.\n\nSecond paragraph."), + ) + .unwrap(); + assert_eq!( + commit.format(), + "docs: update README\n\nFirst paragraph.\n\nSecond paragraph." + ); + } + + /// format() places the body between the header and the breaking-change footer + /// + /// Case: body = Some, footer = Some → "type: desc\n\nbody\n\nBREAKING CHANGE: note" + #[test] + fn format_with_body_and_breaking_change_note() { + let commit = ConventionalCommit::new( + CommitType::Feat, + Scope::empty(), + test_description("drop legacy API"), + "removes legacy endpoint".into(), + Body::from("The endpoint was deprecated in v2."), + ) + .unwrap(); + assert_eq!( + commit.format(), + "feat!: drop legacy API\n\nThe endpoint was deprecated in v2.\n\nBREAKING CHANGE: removes legacy endpoint" + ); + } + + /// format_preview() includes the body in the output + #[test] + fn format_preview_with_body() { + let preview = ConventionalCommit::format_preview( + CommitType::Feat, + &Scope::empty(), + &test_description("add feature"), + &BreakingChange::No, + &Body::from("This explains the change."), + ); + assert_eq!(preview, "feat: add feature\n\nThis explains the change."); + } + + /// format_preview() with body and breaking-change note produces the full message + /// + /// Case: body = Some, footer = Some → "type: desc\n\nbody\n\nBREAKING CHANGE: note" + #[test] + fn format_preview_with_body_and_breaking_change() { + let preview = ConventionalCommit::format_preview( + CommitType::Fix, + &Scope::empty(), + &test_description("drop old API"), + &"old API removed".into(), + &Body::from("Migration guide: see CHANGELOG."), + ); + assert_eq!( + preview, + "fix!: drop old API\n\nMigration guide: see CHANGELOG.\n\nBREAKING CHANGE: old API removed" + ); + } } diff --git a/src/commit/types/mod.rs b/src/commit/types/mod.rs index ee15394..8228840 100644 --- a/src/commit/types/mod.rs +++ b/src/commit/types/mod.rs @@ -13,5 +13,8 @@ pub use scope::{Scope, ScopeError}; mod description; pub use description::{Description, DescriptionError}; +mod body; +pub use body::Body; + mod message; pub use message::{CommitMessageError, ConventionalCommit}; diff --git a/src/lib.rs b/src/lib.rs index dac0a9c..4c110e9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -6,7 +6,7 @@ mod prompts; pub use crate::{ commit::types::{ - BreakingChange, CommitMessageError, CommitType, ConventionalCommit, Description, + Body, BreakingChange, CommitMessageError, CommitType, ConventionalCommit, Description, DescriptionError, Scope, ScopeError, }, error::Error, diff --git a/src/prompts/mock.rs b/src/prompts/mock.rs index a6c8e74..73fe2b6 100644 --- a/src/prompts/mock.rs +++ b/src/prompts/mock.rs @@ -8,7 +8,7 @@ use std::sync::{Arc, Mutex}; use crate::{ - commit::types::{BreakingChange, CommitType, Description, Scope}, + commit::types::{Body, BreakingChange, CommitType, Description, Scope}, error::Error, prompts::prompter::Prompter, }; @@ -20,6 +20,7 @@ enum MockResponse { Scope(Scope), Description(Description), BreakingChange(BreakingChange), + Body(Body), Confirm(bool), Error(Error), } @@ -80,6 +81,15 @@ impl MockPrompts { self } + /// Configure the mock to return a specific body response + pub fn with_body(self, body: Body) -> Self { + self.responses + .lock() + .unwrap() + .push(MockResponse::Body(body)); + self + } + /// Configure the mock to return a specific confirmation response pub fn with_confirm(self, confirm: bool) -> Self { self.responses @@ -130,6 +140,14 @@ impl MockPrompts { .contains(&"input_breaking_change".to_string()) } + /// Check if input_body was called + pub fn was_body_called(&self) -> bool { + self.prompts_called + .lock() + .unwrap() + .contains(&"input_body".to_string()) + } + /// Check if confirm_apply was called pub fn was_confirm_called(&self) -> bool { self.prompts_called @@ -197,6 +215,19 @@ impl Prompter for MockPrompts { } } + fn input_body(&self) -> Result { + self.prompts_called + .lock() + .unwrap() + .push("input_body".to_string()); + + match self.responses.lock().unwrap().remove(0) { + MockResponse::Body(body) => Ok(body), + MockResponse::Error(e) => Err(e), + _ => panic!("MockPrompts: Expected Body response, got different type"), + } + } + fn confirm_apply(&self, _message: &str) -> Result { self.prompts_called .lock() diff --git a/src/prompts/prompter.rs b/src/prompts/prompter.rs index fb6582c..b507eb2 100644 --- a/src/prompts/prompter.rs +++ b/src/prompts/prompter.rs @@ -9,7 +9,7 @@ use inquire::{Confirm, Text}; use unicode_width::UnicodeWidthStr; use crate::{ - commit::types::{BreakingChange, CommitType, Description, Scope}, + commit::types::{Body, BreakingChange, CommitType, Description, Scope}, error::Error, }; @@ -30,6 +30,9 @@ pub trait Prompter: Send + Sync { /// Prompt the user for breaking change fn input_breaking_change(&self) -> Result; + /// Prompt the user to optionally add a free-form body via an external editor + fn input_body(&self) -> Result; + /// Prompt the user to confirm applying the commit message fn confirm_apply(&self, message: &str) -> Result; @@ -176,6 +179,38 @@ impl Prompter for RealPrompts { Ok(trimmed.into()) } + fn input_body(&self) -> Result { + use inquire::Editor; + + let wants_body = Confirm::new("Add a body?") + .with_default(false) + .prompt() + .map_err(|_| Error::Cancelled)?; + + if !wants_body { + return Ok(Body::default()); + } + + let template = "\ +JJ: Body (optional). Markdown is supported.\n\ +JJ: Wrap prose lines at 72 characters where possible.\n\ +JJ: Lines starting with \"JJ:\" will be removed.\n"; + + let raw = Editor::new("Body:") + .with_predefined_text(template) + .with_file_extension(".md") + .prompt() + .map_err(|_| Error::Cancelled)?; + + let stripped: String = raw + .lines() + .filter(|line| !line.starts_with("JJ:")) + .collect::>() + .join("\n"); + + Ok(Body::from(stripped)) + } + fn confirm_apply(&self, message: &str) -> Result { use inquire::Confirm; diff --git a/src/prompts/workflow.rs b/src/prompts/workflow.rs index bfa4e12..597acde 100644 --- a/src/prompts/workflow.rs +++ b/src/prompts/workflow.rs @@ -5,7 +5,8 @@ use crate::{ commit::types::{ - BreakingChange, CommitMessageError, CommitType, ConventionalCommit, Description, Scope, + Body, BreakingChange, CommitMessageError, CommitType, ConventionalCommit, Description, + Scope, }, error::Error, jj::JjExecutor, @@ -62,8 +63,9 @@ impl CommitWorkflow { let scope = self.scope_input().await?; let description = self.description_input().await?; let breaking_change = self.breaking_change_input().await?; + let body = self.body_input().await?; match self - .preview_and_confirm(commit_type, scope, description, breaking_change) + .preview_and_confirm(commit_type, scope, description, breaking_change, body) .await { Ok(conventional_commit) => { @@ -112,6 +114,11 @@ impl CommitWorkflow { self.prompts.input_breaking_change() } + /// Prompt user to optionally add a free-form body via an external editor + async fn body_input(&self) -> Result { + self.prompts.input_body() + } + /// Preview the formatted conventional commit message and get user confirmation /// /// This method also validates that the complete first line @@ -122,10 +129,16 @@ impl CommitWorkflow { scope: Scope, description: Description, breaking_change: BreakingChange, + body: Body, ) -> Result { // Format the message for preview - let message = - ConventionalCommit::format_preview(commit_type, &scope, &description, &breaking_change); + let message = ConventionalCommit::format_preview( + commit_type, + &scope, + &description, + &breaking_change, + &body, + ); // Try to build the conventional commit (this validates the 72-char limit) let conventional_commit: ConventionalCommit = match ConventionalCommit::new( @@ -133,6 +146,7 @@ impl CommitWorkflow { scope.clone(), description.clone(), breaking_change, + body, ) { Ok(cc) => cc, Err(CommitMessageError::FirstLineTooLong { actual, max }) => { @@ -258,8 +272,9 @@ mod tests { let scope = Scope::empty(); let description = Description::parse("test description").unwrap(); let breaking_change = BreakingChange::No; + let body = Body::default(); let result = workflow - .preview_and_confirm(commit_type, scope, description, breaking_change) + .preview_and_confirm(commit_type, scope, description, breaking_change, body) .await; assert!(result.is_ok()); } @@ -305,6 +320,7 @@ mod tests { .with_scope(Scope::empty()) .with_description(Description::parse("add new feature").unwrap()) .with_breaking_change(BreakingChange::Yes) + .with_body(Body::default()) .with_confirm(true); // Create workflow with both mocks @@ -337,6 +353,7 @@ mod tests { .with_scope(Scope::parse("api").unwrap()) .with_description(Description::parse("fix bug").unwrap()) .with_breaking_change(BreakingChange::No) + .with_body(Body::default()) .with_confirm(false); // User cancels at confirmation let workflow = CommitWorkflow::with_prompts(mock_executor, mock_prompts); @@ -360,10 +377,12 @@ mod tests { .with_scope(Scope::parse("very-long-scope-name").unwrap()) .with_description(Description::parse("a".repeat(45)).unwrap()) .with_breaking_change(BreakingChange::No) + .with_body(Body::default()) // Second iteration: short enough to succeed .with_scope(Scope::empty()) .with_description(Description::parse("short description").unwrap()) .with_breaking_change(BreakingChange::No) + .with_body(Body::default()) .with_confirm(true); // Clone before moving into workflow so we can inspect emitted messages after @@ -457,6 +476,7 @@ mod tests { .with_scope(Scope::empty()) .with_description(Description::parse("test").unwrap()) .with_breaking_change(BreakingChange::Yes) + .with_body(Body::default()) .with_confirm(true); let workflow = CommitWorkflow::with_prompts( @@ -479,6 +499,7 @@ mod tests { .with_scope(Scope::empty()) .with_description(Description::parse("test").unwrap()) .with_breaking_change(BreakingChange::Yes) + .with_body(Body::default()) .with_confirm(true); let workflow = CommitWorkflow::with_prompts( @@ -496,6 +517,7 @@ mod tests { .with_scope(Scope::parse("api").unwrap()) .with_description(Description::parse("test").unwrap()) .with_breaking_change(BreakingChange::No) + .with_body(Body::default()) .with_confirm(true); let workflow = CommitWorkflow::with_prompts( @@ -542,6 +564,7 @@ mod tests { Scope::empty(), Description::parse("remove old API").unwrap(), BreakingChange::Yes, + Body::default(), ) .await; @@ -570,6 +593,7 @@ mod tests { Scope::empty(), Description::parse("drop legacy API").unwrap(), breaking_change, + Body::default(), ) .await; @@ -601,6 +625,7 @@ mod tests { .with_scope(Scope::empty()) .with_description(Description::parse("remove old API").unwrap()) .with_breaking_change(BreakingChange::Yes) + .with_body(Body::default()) .with_confirm(true); let workflow = CommitWorkflow::with_prompts(mock_executor, mock_prompts); @@ -620,4 +645,131 @@ mod tests { messages[0], ); } + + // --- Body tests --- + // preview_and_confirm() tests compile now but will fail until the Body::default() + // at line 138 of preview_and_confirm() is replaced with the `body` parameter. + // The full_workflow_* tests additionally require MockPrompts::with_body(). + + /// preview_and_confirm must forward the body to ConventionalCommit::new() + /// + /// Currently the implementation passes Body::default() instead of the + /// received body, so this test will fail until that is fixed. + #[tokio::test] + async fn preview_and_confirm_forwards_body() { + let mock_executor = MockJjExecutor::new(); + let mock_prompts = MockPrompts::new().with_confirm(true); + let workflow = CommitWorkflow::with_prompts(mock_executor, mock_prompts); + + let result = workflow + .preview_and_confirm( + CommitType::Feat, + Scope::empty(), + Description::parse("add feature").unwrap(), + BreakingChange::No, + Body::from("This explains the change."), + ) + .await; + + assert!(result.is_ok(), "expected Ok, got: {:?}", result); + assert!( + result + .unwrap() + .to_string() + .contains("This explains the change."), + "body must appear in the commit message" + ); + } + + /// preview_and_confirm must forward the body even when a breaking change is present + /// + /// Expected format: "type!: desc\n\nbody\n\nBREAKING CHANGE: note" + #[tokio::test] + async fn preview_and_confirm_forwards_body_with_breaking_change() { + let mock_executor = MockJjExecutor::new(); + let mock_prompts = MockPrompts::new().with_confirm(true); + let workflow = CommitWorkflow::with_prompts(mock_executor, mock_prompts); + + let result = workflow + .preview_and_confirm( + CommitType::Feat, + Scope::empty(), + Description::parse("drop legacy API").unwrap(), + "removes legacy endpoint".into(), + Body::from("The endpoint was deprecated in v2."), + ) + .await; + + assert!(result.is_ok(), "expected Ok, got: {:?}", result); + let message = result.unwrap().to_string(); + assert!( + message.contains("The endpoint was deprecated in v2."), + "body must appear in the commit message, got: {message:?}" + ); + assert!( + message.contains("BREAKING CHANGE: removes legacy endpoint"), + "breaking change footer must still be present, got: {message:?}" + ); + } + + /// The full run() workflow must collect a body and include it in the + /// described commit. + /// + /// Requires MockPrompts::with_body() and run() to call body_input(). + #[tokio::test] + async fn full_workflow_describes_commit_with_body() { + let mock_executor = MockJjExecutor::new().with_is_repo_response(Ok(true)); + let mock_prompts = MockPrompts::new() + .with_commit_type(CommitType::Feat) + .with_scope(Scope::empty()) + .with_description(Description::parse("add feature").unwrap()) + .with_breaking_change(BreakingChange::No) + .with_body(Body::from("This explains the change.")) + .with_confirm(true); + + let workflow = CommitWorkflow::with_prompts(mock_executor, mock_prompts); + let result: Result<(), Error> = workflow.run().await; + + assert!( + result.is_ok(), + "expected workflow to succeed, got: {:?}", + result + ); + + let messages = workflow.executor.describe_messages(); + assert_eq!(messages.len(), 1, "expected exactly one describe() call"); + assert!( + messages[0].contains("This explains the change."), + "body must appear in the described commit, got: {:?}", + messages[0] + ); + } + + /// run() must still work correctly when the user declines to add a body + /// + /// Requires MockPrompts::with_body() returning Body::default(). + #[tokio::test] + async fn full_workflow_with_no_body_succeeds() { + let mock_executor = MockJjExecutor::new().with_is_repo_response(Ok(true)); + let mock_prompts = MockPrompts::new() + .with_commit_type(CommitType::Fix) + .with_scope(Scope::empty()) + .with_description(Description::parse("fix crash").unwrap()) + .with_breaking_change(BreakingChange::No) + .with_body(Body::default()) + .with_confirm(true); + + let workflow = CommitWorkflow::with_prompts(mock_executor, mock_prompts); + let result: Result<(), Error> = workflow.run().await; + + assert!( + result.is_ok(), + "expected workflow to succeed, got: {:?}", + result + ); + + let messages = workflow.executor.describe_messages(); + assert_eq!(messages.len(), 1); + assert_eq!(messages[0], "fix: fix crash"); + } } diff --git a/tests/cli.rs b/tests/cli.rs index 45724e2..8ab6e19 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -1,6 +1,6 @@ use assert_fs::TempDir; #[cfg(feature = "test-utils")] -use jj_cz::{BreakingChange, CommitType, Description, MockPrompts, Scope}; +use jj_cz::{Body, BreakingChange, CommitType, Description, MockPrompts, Scope}; use jj_cz::{CommitWorkflow, Error, JjLib}; #[cfg(feature = "test-utils")] use jj_lib::{config::StackedConfig, settings::UserSettings, workspace::Workspace}; @@ -28,6 +28,7 @@ async fn test_happy_path_integration() { .with_scope(Scope::empty()) .with_description(Description::parse("add new feature").unwrap()) .with_breaking_change(BreakingChange::No) + .with_body(Body::default()) .with_confirm(true); // Create a mock executor that tracks calls @@ -87,6 +88,7 @@ async fn test_cancellation() { .with_scope(Scope::empty()) .with_description(Description::parse("test").unwrap()) .with_breaking_change(BreakingChange::No) + .with_body(Body::default()) .with_confirm(true); let workflow = CommitWorkflow::with_prompts(executor, mock_prompts);