feat: implement breaking change input
This commit is contained in:
@@ -8,7 +8,7 @@
|
||||
use std::sync::{Arc, Mutex};
|
||||
|
||||
use crate::{
|
||||
commit::types::{CommitType, Description, Scope},
|
||||
commit::types::{BreakingChange, CommitType, Description, Scope},
|
||||
error::Error,
|
||||
prompts::prompter::Prompter,
|
||||
};
|
||||
@@ -19,6 +19,7 @@ enum MockResponse {
|
||||
CommitType(CommitType),
|
||||
Scope(Scope),
|
||||
Description(Description),
|
||||
BreakingChange(BreakingChange),
|
||||
Confirm(bool),
|
||||
Error(Error),
|
||||
}
|
||||
@@ -70,6 +71,15 @@ impl MockPrompts {
|
||||
self
|
||||
}
|
||||
|
||||
/// Configure the mock to return a specific breaking change response
|
||||
pub fn with_breaking_change(self, breaking_change: BreakingChange) -> Self {
|
||||
self.responses
|
||||
.lock()
|
||||
.unwrap()
|
||||
.push(MockResponse::BreakingChange(breaking_change));
|
||||
self
|
||||
}
|
||||
|
||||
/// Configure the mock to return a specific confirmation response
|
||||
pub fn with_confirm(self, confirm: bool) -> Self {
|
||||
self.responses
|
||||
@@ -112,6 +122,14 @@ impl MockPrompts {
|
||||
.contains(&"input_description".to_string())
|
||||
}
|
||||
|
||||
/// Check if input_breaking_change was called
|
||||
pub fn was_breaking_change_called(&self) -> bool {
|
||||
self.prompts_called
|
||||
.lock()
|
||||
.unwrap()
|
||||
.contains(&"input_breaking_change".to_string())
|
||||
}
|
||||
|
||||
/// Check if confirm_apply was called
|
||||
pub fn was_confirm_called(&self) -> bool {
|
||||
self.prompts_called
|
||||
@@ -166,6 +184,19 @@ impl Prompter for MockPrompts {
|
||||
}
|
||||
}
|
||||
|
||||
fn input_breaking_change(&self) -> Result<BreakingChange, Error> {
|
||||
self.prompts_called
|
||||
.lock()
|
||||
.unwrap()
|
||||
.push("input_breaking_change".to_string());
|
||||
|
||||
match self.responses.lock().unwrap().remove(0) {
|
||||
MockResponse::BreakingChange(bc) => Ok(bc),
|
||||
MockResponse::Error(e) => Err(e),
|
||||
_ => panic!("MockPrompts: Expected BreakingChange response, got different type"),
|
||||
}
|
||||
}
|
||||
|
||||
fn confirm_apply(&self, _message: &str) -> Result<bool, Error> {
|
||||
self.prompts_called
|
||||
.lock()
|
||||
@@ -281,4 +312,64 @@ mod tests {
|
||||
let mock = MockPrompts::new();
|
||||
assert!(mock.emitted_messages().is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mock_input_breaking_change_no() {
|
||||
let mock = MockPrompts::new().with_breaking_change(BreakingChange::No);
|
||||
let result = mock.input_breaking_change();
|
||||
assert!(result.is_ok());
|
||||
assert_eq!(result.unwrap(), BreakingChange::No);
|
||||
assert!(mock.was_breaking_change_called());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mock_input_breaking_change_yes_no_note() {
|
||||
let mock = MockPrompts::new().with_breaking_change(BreakingChange::Yes);
|
||||
let result = mock.input_breaking_change();
|
||||
assert!(result.is_ok());
|
||||
assert_eq!(result.unwrap(), BreakingChange::Yes);
|
||||
assert!(mock.was_breaking_change_called());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mock_input_breaking_change_yes_with_note() {
|
||||
let mock = MockPrompts::new().with_breaking_change("removes old API".into());
|
||||
let result = mock.input_breaking_change();
|
||||
assert!(result.is_ok());
|
||||
assert_eq!(
|
||||
result.unwrap(),
|
||||
BreakingChange::WithNote("removes old API".into())
|
||||
);
|
||||
assert!(mock.was_breaking_change_called());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mock_input_breaking_change_error() {
|
||||
let mock = MockPrompts::new().with_error(Error::Cancelled);
|
||||
let result = mock.input_breaking_change();
|
||||
assert!(result.is_err());
|
||||
assert!(matches!(result.unwrap_err(), Error::Cancelled));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mock_tracks_breaking_change_call() {
|
||||
let mock = MockPrompts::new()
|
||||
.with_commit_type(CommitType::Fix)
|
||||
.with_scope(Scope::empty())
|
||||
.with_description(Description::parse("test").unwrap())
|
||||
.with_breaking_change(BreakingChange::No)
|
||||
.with_confirm(true);
|
||||
|
||||
mock.select_commit_type().unwrap();
|
||||
mock.input_scope().unwrap();
|
||||
mock.input_description().unwrap();
|
||||
mock.input_breaking_change().unwrap();
|
||||
mock.confirm_apply("test").unwrap();
|
||||
|
||||
assert!(mock.was_commit_type_called());
|
||||
assert!(mock.was_scope_called());
|
||||
assert!(mock.was_description_called());
|
||||
assert!(mock.was_breaking_change_called());
|
||||
assert!(mock.was_confirm_called());
|
||||
}
|
||||
}
|
||||
|
||||
@@ -5,8 +5,10 @@
|
||||
//! [`CommitWorkflow`](super::CommitWorkflow) to use real interactive prompts
|
||||
//! in production while accepting mock implementations in tests.
|
||||
|
||||
use inquire::{Confirm, Text};
|
||||
|
||||
use crate::{
|
||||
commit::types::{CommitType, Description, Scope},
|
||||
commit::types::{BreakingChange, CommitType, Description, Scope},
|
||||
error::Error,
|
||||
};
|
||||
|
||||
@@ -24,6 +26,9 @@ pub trait Prompter: Send + Sync {
|
||||
/// Prompt the user to input a required description
|
||||
fn input_description(&self) -> Result<Description, Error>;
|
||||
|
||||
/// Prompt the user for breaking change
|
||||
fn input_breaking_change(&self) -> Result<BreakingChange, Error>;
|
||||
|
||||
/// Prompt the user to confirm applying the commit message
|
||||
fn confirm_apply(&self, message: &str) -> Result<bool, Error>;
|
||||
|
||||
@@ -34,6 +39,18 @@ pub trait Prompter: Send + Sync {
|
||||
fn emit_message(&self, msg: &str);
|
||||
}
|
||||
|
||||
fn format_message_box(message: &str) -> String {
|
||||
let preview_width = 72 + 2; // max width + space padding
|
||||
let mut lines: Vec<String> = Vec::new();
|
||||
lines.push(format!("┌{}┐", "─".repeat(preview_width)));
|
||||
for line in message.split("\n") {
|
||||
let padding = 72_usize.saturating_sub(line.chars().count());
|
||||
lines.push(format!("│ {line}{:padding$} │", ""));
|
||||
}
|
||||
lines.push(format!("└{}┘", "─".repeat(preview_width)));
|
||||
lines.join("\n")
|
||||
}
|
||||
|
||||
/// Production implementation of [`Prompter`] using the `inquire` crate
|
||||
#[derive(Debug)]
|
||||
pub struct RealPrompts;
|
||||
@@ -137,18 +154,30 @@ impl Prompter for RealPrompts {
|
||||
}
|
||||
}
|
||||
|
||||
fn input_breaking_change(&self) -> Result<BreakingChange, Error> {
|
||||
if !Confirm::new("Does this revision include a breaking change?")
|
||||
.with_default(false)
|
||||
.prompt()
|
||||
.map_err(|_| Error::Cancelled)?
|
||||
{
|
||||
return Ok(BreakingChange::No);
|
||||
}
|
||||
let answer = Text::new("Enter the description of the breaking change:")
|
||||
.with_help_message("Enter an empty message to skip creating a message footer")
|
||||
.prompt()
|
||||
.map_err(|_| Error::Cancelled)?;
|
||||
let trimmed = answer.trim();
|
||||
Ok(trimmed.into())
|
||||
}
|
||||
|
||||
fn confirm_apply(&self, message: &str) -> Result<bool, Error> {
|
||||
use inquire::Confirm;
|
||||
|
||||
// Show preview
|
||||
println!();
|
||||
println!("📝 Commit Message Preview:");
|
||||
println!("┌────────────────────────────────────────────────────────────────────────┐");
|
||||
// Pad with spaces to fill the box
|
||||
let padding = 72_usize.saturating_sub(message.chars().count()) - 1;
|
||||
println!("│ {message}{:padding$}│", "");
|
||||
println!("└────────────────────────────────────────────────────────────────────────┘");
|
||||
println!();
|
||||
println!(
|
||||
"\n📝 Commit Message Preview:\n{}\n",
|
||||
format_message_box(message)
|
||||
);
|
||||
|
||||
// Get confirmation
|
||||
Confirm::new("Apply this commit message?")
|
||||
@@ -174,4 +203,71 @@ mod tests {
|
||||
fn _accepts_prompter(_p: impl Prompter) {}
|
||||
_accepts_prompter(real);
|
||||
}
|
||||
|
||||
/// Top border uses exactly preview_width (74) dashes; bottom likewise
|
||||
#[test]
|
||||
fn format_message_box_borders() {
|
||||
let result = format_message_box("hello");
|
||||
let lines: Vec<&str> = result.split('\n').collect();
|
||||
let dashes = "─".repeat(74);
|
||||
assert_eq!(lines[0], format!("┌{dashes}┐"));
|
||||
assert_eq!(lines[lines.len() - 1], format!("└{dashes}┘"));
|
||||
}
|
||||
|
||||
/// A single-line message produces exactly 3 rows: top, content, bottom
|
||||
#[test]
|
||||
fn format_message_box_single_line_row_count() {
|
||||
let result = format_message_box("feat: add login");
|
||||
assert_eq!(result.split('\n').count(), 3);
|
||||
}
|
||||
|
||||
/// A message with one `\n` produces 4 rows: top, two content, bottom
|
||||
#[test]
|
||||
fn format_message_box_multi_line_row_count() {
|
||||
let result = format_message_box("feat: add login\nsecond line");
|
||||
assert_eq!(result.split('\n').count(), 4);
|
||||
}
|
||||
|
||||
/// A breaking-change message (`\n\n`) produces an empty content row for the blank line
|
||||
#[test]
|
||||
fn format_message_box_blank_separator_line() {
|
||||
let msg = "feat!: drop old API\n\nBREAKING CHANGE: removed";
|
||||
let result = format_message_box(msg);
|
||||
assert_eq!(result.split('\n').count(), 5); // top + 3 content + bottom
|
||||
}
|
||||
|
||||
/// All output rows have identical char counts (the box is rectangular)
|
||||
#[test]
|
||||
fn format_message_box_all_rows_same_width() {
|
||||
let msg = "feat(auth): add login\n\nBREAKING CHANGE: old API removed";
|
||||
let result = format_message_box(msg);
|
||||
let widths: Vec<usize> = result.split('\n').map(|l| l.chars().count()).collect();
|
||||
let expected = widths[0];
|
||||
assert!(
|
||||
widths.iter().all(|&w| w == expected),
|
||||
"rows have differing widths: {:?}",
|
||||
widths
|
||||
);
|
||||
}
|
||||
|
||||
/// An empty message produces a single fully-padded content row
|
||||
#[test]
|
||||
fn format_message_box_empty_message() {
|
||||
let result = format_message_box("");
|
||||
let lines: Vec<&str> = result.split('\n').collect();
|
||||
assert_eq!(lines.len(), 3);
|
||||
// "│ " + 72 spaces + " │" = 76 chars
|
||||
let expected = format!("│ {:72} │", "");
|
||||
assert_eq!(lines[1], expected);
|
||||
}
|
||||
|
||||
/// A line of exactly 72 characters leaves no right-hand padding
|
||||
#[test]
|
||||
fn format_message_box_line_exactly_72_chars() {
|
||||
let line_72 = "a".repeat(72);
|
||||
let result = format_message_box(&line_72);
|
||||
let lines: Vec<&str> = result.split('\n').collect();
|
||||
let expected = format!("│ {line_72} │");
|
||||
assert_eq!(lines[1], expected);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -4,7 +4,9 @@
|
||||
//! creating a conventional commit message using interactive prompts.
|
||||
|
||||
use crate::{
|
||||
commit::types::{CommitMessageError, CommitType, ConventionalCommit, Description, Scope},
|
||||
commit::types::{
|
||||
BreakingChange, CommitMessageError, CommitType, ConventionalCommit, Description, Scope,
|
||||
},
|
||||
error::Error,
|
||||
jj::JjExecutor,
|
||||
prompts::prompter::{Prompter, RealPrompts},
|
||||
@@ -52,30 +54,19 @@ impl<J: JjExecutor, P: Prompter> CommitWorkflow<J, P> {
|
||||
/// - Repository operation fails
|
||||
/// - Message validation fails
|
||||
pub async fn run(&self) -> Result<(), Error> {
|
||||
// Verify we're in a jj repository
|
||||
if !self.executor.is_repository().await? {
|
||||
return Err(Error::NotARepository);
|
||||
}
|
||||
|
||||
// Step 1: Select commit type (kept across retries)
|
||||
let commit_type = self.type_selection().await?;
|
||||
|
||||
// Steps 2–4 loop: re-prompt scope and description when the combined
|
||||
// first line would exceed 72 characters (issue 3.4).
|
||||
loop {
|
||||
// Step 2: Input scope (optional)
|
||||
let scope = self.scope_input().await?;
|
||||
|
||||
// Step 3: Input description (required)
|
||||
let description = self.description_input().await?;
|
||||
|
||||
// Step 4: Preview and confirm
|
||||
let breaking_change = self.breaking_change_input().await?;
|
||||
match self
|
||||
.preview_and_confirm(commit_type, scope, description)
|
||||
.preview_and_confirm(commit_type, scope, description, breaking_change)
|
||||
.await
|
||||
{
|
||||
Ok(conventional_commit) => {
|
||||
// Step 5: Apply the message
|
||||
self.executor
|
||||
.describe(&conventional_commit.to_string())
|
||||
.await?;
|
||||
@@ -99,35 +90,49 @@ impl<J: JjExecutor, P: Prompter> CommitWorkflow<J, P> {
|
||||
|
||||
/// Prompt user to input an optional scope
|
||||
///
|
||||
/// Returns Ok(Scope) with the validated scope, or Error::Cancelled if user cancels
|
||||
/// Returns Ok(Scope) with the validated scope, or
|
||||
/// Error::Cancelled if user cancels
|
||||
async fn scope_input(&self) -> Result<Scope, Error> {
|
||||
self.prompts.input_scope()
|
||||
}
|
||||
|
||||
/// Prompt user to input a required description
|
||||
///
|
||||
/// Returns Ok(Description) with the validated description, or Error::Cancelled if user cancels
|
||||
/// Returns Ok(Description) with the validated description, or
|
||||
/// Error::Cancelled if user cancels
|
||||
async fn description_input(&self) -> Result<Description, Error> {
|
||||
self.prompts.input_description()
|
||||
}
|
||||
|
||||
/// Prompt user for breaking change
|
||||
///
|
||||
/// Returns Ok(BreakingChange) with the validated breaking change,
|
||||
/// or Error::Cancel if user cancels
|
||||
async fn breaking_change_input(&self) -> Result<BreakingChange, Error> {
|
||||
self.prompts.input_breaking_change()
|
||||
}
|
||||
|
||||
/// Preview the formatted conventional commit message and get user confirmation
|
||||
///
|
||||
/// This method also validates that the complete first line doesn't exceed 72 characters
|
||||
/// This method also validates that the complete first line
|
||||
/// doesn't exceed 72 characters
|
||||
async fn preview_and_confirm(
|
||||
&self,
|
||||
commit_type: CommitType,
|
||||
scope: Scope,
|
||||
description: Description,
|
||||
breaking_change: BreakingChange,
|
||||
) -> Result<ConventionalCommit, Error> {
|
||||
// Format the message for preview
|
||||
let message = ConventionalCommit::format_preview(commit_type, &scope, &description);
|
||||
let message =
|
||||
ConventionalCommit::format_preview(commit_type, &scope, &description, &breaking_change);
|
||||
|
||||
// Try to build the conventional commit (this validates the 72-char limit)
|
||||
let conventional_commit: ConventionalCommit = match ConventionalCommit::new(
|
||||
commit_type,
|
||||
scope.clone(),
|
||||
description.clone(),
|
||||
breaking_change,
|
||||
) {
|
||||
Ok(cc) => cc,
|
||||
Err(CommitMessageError::FirstLineTooLong { actual, max }) => {
|
||||
@@ -252,9 +257,9 @@ mod tests {
|
||||
let commit_type = CommitType::Feat;
|
||||
let scope = Scope::empty();
|
||||
let description = Description::parse("test description").unwrap();
|
||||
|
||||
let breaking_change = BreakingChange::No;
|
||||
let result = workflow
|
||||
.preview_and_confirm(commit_type, scope, description)
|
||||
.preview_and_confirm(commit_type, scope, description, breaking_change)
|
||||
.await;
|
||||
assert!(result.is_ok());
|
||||
}
|
||||
@@ -299,6 +304,7 @@ mod tests {
|
||||
.with_commit_type(CommitType::Feat)
|
||||
.with_scope(Scope::empty())
|
||||
.with_description(Description::parse("add new feature").unwrap())
|
||||
.with_breaking_change(BreakingChange::Yes)
|
||||
.with_confirm(true);
|
||||
|
||||
// Create workflow with both mocks
|
||||
@@ -330,6 +336,7 @@ mod tests {
|
||||
.with_commit_type(CommitType::Fix)
|
||||
.with_scope(Scope::parse("api").unwrap())
|
||||
.with_description(Description::parse("fix bug").unwrap())
|
||||
.with_breaking_change(BreakingChange::No)
|
||||
.with_confirm(false); // User cancels at confirmation
|
||||
|
||||
let workflow = CommitWorkflow::with_prompts(mock_executor, mock_prompts);
|
||||
@@ -352,9 +359,11 @@ mod tests {
|
||||
// First iteration: scope + description exceed 72 chars combined
|
||||
.with_scope(Scope::parse("very-long-scope-name").unwrap())
|
||||
.with_description(Description::parse("a".repeat(45)).unwrap())
|
||||
.with_breaking_change(BreakingChange::No)
|
||||
// Second iteration: short enough to succeed
|
||||
.with_scope(Scope::empty())
|
||||
.with_description(Description::parse("short description").unwrap())
|
||||
.with_breaking_change(BreakingChange::No)
|
||||
.with_confirm(true);
|
||||
|
||||
// Clone before moving into workflow so we can inspect emitted messages after
|
||||
@@ -447,6 +456,7 @@ mod tests {
|
||||
.with_commit_type(*commit_type)
|
||||
.with_scope(Scope::empty())
|
||||
.with_description(Description::parse("test").unwrap())
|
||||
.with_breaking_change(BreakingChange::Yes)
|
||||
.with_confirm(true);
|
||||
|
||||
let workflow = CommitWorkflow::with_prompts(
|
||||
@@ -468,6 +478,7 @@ mod tests {
|
||||
.with_commit_type(CommitType::Feat)
|
||||
.with_scope(Scope::empty())
|
||||
.with_description(Description::parse("test").unwrap())
|
||||
.with_breaking_change(BreakingChange::Yes)
|
||||
.with_confirm(true);
|
||||
|
||||
let workflow = CommitWorkflow::with_prompts(
|
||||
@@ -484,6 +495,7 @@ mod tests {
|
||||
.with_commit_type(CommitType::Feat)
|
||||
.with_scope(Scope::parse("api").unwrap())
|
||||
.with_description(Description::parse("test").unwrap())
|
||||
.with_breaking_change(BreakingChange::No)
|
||||
.with_confirm(true);
|
||||
|
||||
let workflow = CommitWorkflow::with_prompts(
|
||||
@@ -509,4 +521,99 @@ mod tests {
|
||||
let workflow = CommitWorkflow::with_prompts(mock_executor, mock_prompts);
|
||||
assert!(matches!(workflow, CommitWorkflow { .. }));
|
||||
}
|
||||
|
||||
/// Preview_and_confirm must forward BreakingChange::Yes to
|
||||
/// ConventionalCommit::new(), producing a commit whose string
|
||||
/// contains '!'.
|
||||
///
|
||||
/// Before the fix the parameter was ignored and
|
||||
/// BreakingChange::No was hard-coded, so a confirmed
|
||||
/// breaking-change commit was silently applied without the '!'
|
||||
/// marker.
|
||||
#[tokio::test]
|
||||
async fn preview_and_confirm_forwards_breaking_change_yes() {
|
||||
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("remove old API").unwrap(),
|
||||
BreakingChange::Yes,
|
||||
)
|
||||
.await;
|
||||
|
||||
assert!(result.is_ok(), "expected Ok, got: {:?}", result);
|
||||
let message = result.unwrap().to_string();
|
||||
assert!(
|
||||
message.contains("feat!:"),
|
||||
"expected '!' marker in described message, got: {:?}",
|
||||
message,
|
||||
);
|
||||
}
|
||||
|
||||
/// Preview_and_confirm must forward BreakingChange::WithNote,
|
||||
/// producing a commit with both the '!' header marker and the
|
||||
/// BREAKING CHANGE footer.
|
||||
#[tokio::test]
|
||||
async fn preview_and_confirm_forwards_breaking_change_with_note() {
|
||||
let mock_executor = MockJjExecutor::new();
|
||||
let mock_prompts = MockPrompts::new().with_confirm(true);
|
||||
let workflow = CommitWorkflow::with_prompts(mock_executor, mock_prompts);
|
||||
|
||||
let breaking_change: BreakingChange = "removes legacy endpoint".into();
|
||||
let result = workflow
|
||||
.preview_and_confirm(
|
||||
CommitType::Feat,
|
||||
Scope::empty(),
|
||||
Description::parse("drop legacy API").unwrap(),
|
||||
breaking_change,
|
||||
)
|
||||
.await;
|
||||
|
||||
assert!(result.is_ok(), "expected Ok, got: {:?}", result);
|
||||
let message = result.unwrap().to_string();
|
||||
assert!(
|
||||
message.contains("feat!:"),
|
||||
"expected '!' header marker in message, got: {:?}",
|
||||
message,
|
||||
);
|
||||
assert!(
|
||||
message.contains("BREAKING CHANGE:"),
|
||||
"expected BREAKING CHANGE footer in message, got: {:?}",
|
||||
message,
|
||||
);
|
||||
}
|
||||
|
||||
/// The message passed to executor.describe() must include the '!'
|
||||
/// marker when the user selects a breaking change.
|
||||
///
|
||||
/// This test exercises the full run() path and inspects what was
|
||||
/// actually handed to the jj executor, which is the authoritative
|
||||
/// check that the described commit is correct.
|
||||
#[tokio::test]
|
||||
async fn full_workflow_describes_commit_with_breaking_change_marker() {
|
||||
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("remove old API").unwrap())
|
||||
.with_breaking_change(BreakingChange::Yes)
|
||||
.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("feat!:"),
|
||||
"expected '!' marker in the described message, got: {:?}",
|
||||
messages[0],
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user