Basic conventional commit builder #3
Conversation
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as Commit Command
participant Builder as Message Builder
participant Stdout as Output
User->>CLI: run `app commit` with flags (--type, --scope, --title, --body, --breaking, --issue)
CLI->>CLI: parse flags → conventional.Commit
CLI->>Builder: BuildCommitMessage(commit)
Builder->>Builder: validate Type & Title
alt validation fails
Builder-->>CLI: ErrRequiredPartNotPreset
CLI-->>User: print error
else validation succeeds
Builder->>Builder: buildHeader(Type, Scope, Breaking, Title)
Builder->>Builder: buildFooter(Breaking, Issue, Body)
Builder->>Builder: concat header + newline + footer
Builder-->>CLI: formatted message
CLI->>Stdout: print message
CLI-->>User: exit success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
internal/conventional/commit.go (1)
3-11: Consider adding documentation for the Commit struct.Adding a comment describing the purpose of the struct and its fields would improve maintainability and help users understand the conventional commit structure.
Apply this diff to add documentation:
+// Commit represents the components of a Conventional Commit message. +// See https://www.conventionalcommits.org/ for the specification. type ( Commit struct { + // Type describes the kind of change (e.g., "feat", "fix", "docs") Type string + // Scope is an optional context for the change (e.g., "api", "cli") Scope string + // Title is a short description of the change Title string + // Body is an optional longer description Body string + // Footer is optional metadata (e.g., "BREAKING CHANGE:", issue refs) Footer string } )cmd/commit.go (3)
26-32: Consider adding Required and Usage fields to flags.The flags would benefit from descriptions and required markers to improve the CLI user experience. This helps users understand what each flag does without reading the code.
Apply this diff to improve flag definitions:
Flags: []cli.Flag{ - &cli.StringFlag{Name: "type", OnlyOnce: true, Destination: &commitType}, - &cli.StringFlag{Name: "scope", OnlyOnce: true, Destination: &scope}, - &cli.StringFlag{Name: "title", OnlyOnce: true, Destination: &title}, - &cli.StringFlag{Name: "body", OnlyOnce: true, Destination: &body}, - &cli.StringFlag{Name: "footer", OnlyOnce: true, Destination: &footer}, + &cli.StringFlag{Name: "type", OnlyOnce: true, Destination: &commitType, Required: true, Usage: "Type of change (e.g., feat, fix, docs)"}, + &cli.StringFlag{Name: "scope", OnlyOnce: true, Destination: &scope, Usage: "Optional scope of the change"}, + &cli.StringFlag{Name: "title", OnlyOnce: true, Destination: &title, Required: true, Usage: "Short description of the change"}, + &cli.StringFlag{Name: "body", OnlyOnce: true, Destination: &body, Usage: "Optional longer description"}, + &cli.StringFlag{Name: "footer", OnlyOnce: true, Destination: &footer, Usage: "Optional footer (e.g., breaking changes, issue refs)"}, },
34-39: Remove duplicate validation.The validation for
commitTypeandtitleis redundant sinceBuildCommitalready validates these fields (lines 13-19 ininternal/conventional/build_commit.go). Removing this duplicate code simplifies maintenance and follows the single responsibility principle.Apply this diff to remove duplicate validation:
Action: func(_ context.Context, _ *cli.Command) error { - if commitType == "" { - return fmt.Errorf("%w: commitType must be provided", ErrInvalidFlag) - } - if title == "" { - return fmt.Errorf("%w: title must be provided", ErrInvalidFlag) - } - commit, err := conventional.BuildCommit(&conventional.Commit{Alternatively, if you prefer validation at the CLI layer, consider marking the flags as
Required: true(see previous comment) and removing validation fromBuildCommitinstead.
52-52: Usefmt.Printlninstead oflog.Printlnfor output.Using
log.Printlnadds timestamp and prefix information to the output, which is not appropriate for displaying the generated commit message. The output should be clean text that can be piped or copied.Apply this diff to fix the output:
- log.Println(commit) + fmt.Println(commit)You can also remove the unused
logimport after this change:import ( "context" "errors" "fmt" - "log" "github.com/urfave/cli/v3"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.coderabbit.yaml(1 hunks).lefthook.yml(1 hunks)cmd/commands.go(1 hunks)cmd/commit.go(1 hunks)internal/conventional/build_commit.go(1 hunks)internal/conventional/commit.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/commit.go (2)
internal/conventional/build_commit.go (1)
BuildCommit(10-36)internal/conventional/commit.go (1)
Commit(4-10)
internal/conventional/build_commit.go (1)
internal/conventional/commit.go (1)
Commit(4-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
.lefthook.yml (1)
39-48: LGTM! Good pre-commit hook structure.The nested group structure with sequential execution ensures that formatting is applied before linting runs, and
stage_fixed: trueautomatically stages any formatting changes. This is a clean and effective approach..coderabbit.yaml (1)
1-25: LGTM! Valid CodeRabbit configuration.The configuration settings are all valid according to the schema and establish a reasonable automated review workflow with pre-merge title checks enforced as errors.
cmd/commands.go (1)
7-10: LGTM! Clean integration of the new command.The commit command is properly added to the commands array, following the existing pattern.
Change footer for issue and breaking in data structure, rework message generation, add more params to flags
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmd/commit.go (1)
28-31: Consider simplifying stdout error handling.While not incorrect, checking the error from
fmt.Printlnwhen writing to stdout is overly defensive. In practice, writing to stdout rarely fails, and the application would likely have bigger problems if it did.If you prefer simpler code, you could use:
- _, err = fmt.Println(commitMessage) - if err != nil { - return fmt.Errorf("error printing built commit message: %w", err) - } - - return nil + fmt.Println(commitMessage) + return nilHowever, the current error handling is also acceptable if you prefer defensive programming.
internal/conventional/message.go (1)
47-69: Consider simplifying the footer assembly logic.The current implementation is functionally correct and handles all edge cases properly. However, the logic is quite complex, especially lines 58-66, which makes it harder to understand and maintain.
Here's a clearer approach that builds the footer in logical order:
func buildFooter(commit *Commit) string { - footer := "" + var parts []string + if commit.Body != "" { + parts = append(parts, commit.Body) + } + if commit.BreakingChange != "" { - footer += newLine + "BREAKING CHANGE: " + commit.BreakingChange + parts = append(parts, "BREAKING CHANGE: "+commit.BreakingChange) } if commit.Issue != "" { - footer += newLine + "Issue: " + commit.Issue + parts = append(parts, "Issue: "+commit.Issue) } - if commit.Body != "" { - if footer != "" { - footer = newLine + commit.Body + newLine + footer + newLine - } else { - footer = newLine + commit.Body + newLine - } - } else if footer != "" { - footer += newLine + if len(parts) == 0 { + return "" } - return footer + return newLine + strings.Join(parts, newLine+newLine) + newLine }This approach:
- Collects all parts in order (body, breaking change, issue)
- Joins them with double newlines (blank line between sections)
- Adds leading and trailing newlines
- Is easier to understand and modify
Note: You'll need to add
"strings"to the imports.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/commit.go(1 hunks)internal/conventional/commit.go(1 hunks)internal/conventional/message.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/commit.go (2)
internal/conventional/commit.go (1)
Commit(6-19)internal/conventional/message.go (1)
BuildCommitMessage(16-29)
internal/conventional/message.go (1)
internal/conventional/commit.go (1)
Commit(6-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
internal/conventional/commit.go (1)
3-20: LGTM!The Commit struct is well-designed with clear documentation and appropriate field types for representing Conventional Commit components.
cmd/commit.go (2)
38-83: LGTM!The flag definitions are well-structured with appropriate required/optional markings, clear usage descriptions, and proper pointer destinations.
13-13: Remove unused error variable.
ErrInvalidFlagis declared but never used in this file. The urfave/cli framework handles required flag validation automatically, making this error variable unnecessary.Apply this diff to remove the unused variable:
-var ErrInvalidFlag = errors.New("invalid flag") -Likely an incorrect or invalid review comment.
internal/conventional/message.go (3)
8-13: LGTM!The constant and error variable are well-defined. The
newLineconstant is used consistently throughout the file, which aids maintainability.
15-29: LGTM!The validation logic correctly enforces required fields (type and title) per the Conventional Commits specification, and error messages provide clear context.
31-45: LGTM!The header formatting correctly implements the Conventional Commits specification with proper handling of optional scope and breaking change marker.
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/conventional/message_test.go (2)
10-16: Minor style note: simplify type declaration.The type declaration uses parentheses for a single type, which is typically reserved for grouping multiple types. Consider simplifying to:
-type ( - testCases struct { - commitData *Commit - err error - result string - } -) +type testCases struct { + commitData *Commit + err error + result string +}
98-110: Consider more descriptive test names.The tests use numeric indices (
strconv.Itoa(i)) for naming, which makes it harder to identify failing tests at a glance. Consider adding descriptive names to each test case and using them fort.Run().Example:
type testCases struct { name string commitData *Commit err error result string } // Then in the test: t.Run(tt.name, func(t *testing.T) { // ... })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/commit.go(1 hunks)internal/conventional/message_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/commit.go (2)
internal/conventional/commit.go (1)
Commit(6-19)internal/conventional/message.go (1)
BuildCommitMessage(16-29)
internal/conventional/message_test.go (2)
internal/conventional/commit.go (1)
Commit(6-19)internal/conventional/message.go (2)
ErrRequiredPartNotPreset(13-13)BuildCommitMessage(16-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: build
🔇 Additional comments (3)
cmd/commit.go (2)
15-36: LGTM! Previous shadowing issue resolved.The variable shadowing issue from the previous review has been fixed. The code now uses distinct variable names (
commitDatafor the struct,messagefor the built string), making it clear and maintainable. The Action function correctly builds and prints the commit message with appropriate error handling.
38-83: LGTM!The flag definitions are well-structured and correctly map to the
Commitstruct fields. The required flags (type and title) align with the validation inBuildCommitMessage, and the usage descriptions are clear and helpful.internal/conventional/message_test.go (1)
18-96: LGTM!The test cases provide comprehensive coverage of the
BuildCommitMessagefunction, including:
- Various combinations of optional fields (scope, body, issue, breaking changes)
- Error cases for missing required fields (type and title)
The expected output strings correctly follow the conventional commit format.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.