Feature/more consistent stdout #66

noe.trevino · 2026-02-12T00:59:26-06:00

noe.trevino commented

2026-02-12 00:59:26 -06:00

No description provided.

claude[bot] (Migrated from github.com) reviewed

2026-02-12 01:00:25 -06:00

internal/cli/assignee/delete.go

					
				@ -105,6 +105,6 @@ func runDelete(cmd *cobra.Command, args []string) error {

						})

					}

DRY violation: cli.GetColorScheme() is called repeatedly in every file. This pattern is repeated across 13 different files in this PR. Consider extracting this into a helper function or method on the CLI instance to eliminate this duplication.

Example refactoring:

func (c *CLI) RenderSuccess(message string) {
    fmt.Print(styles.RenderSuccess(message, c.GetColorScheme()))
}

Then each call site becomes:

cliInstance.RenderSuccess(fmt.Sprintf("Assignee %d deleted successfully", assigneeID))

This would eliminate ~26 lines of duplicate colorScheme := cli.GetColorScheme() declarations.

DRY violation: `cli.GetColorScheme()` is called repeatedly in every file. This pattern is repeated across 13 different files in this PR. Consider extracting this into a helper function or method on the CLI instance to eliminate this duplication. Example refactoring: ```go func (c *CLI) RenderSuccess(message string) { fmt.Print(styles.RenderSuccess(message, c.GetColorScheme())) } ``` Then each call site becomes: ```go cliInstance.RenderSuccess(fmt.Sprintf("Assignee %d deleted successfully", assigneeID)) ``` This would eliminate ~26 lines of duplicate `colorScheme := cli.GetColorScheme()` declarations.

claude[bot] (Migrated from github.com) reviewed

2026-02-12 01:00:27 -06:00

internal/cli/setup/claude.go

The colorScheme variable is declared once at line 104, but then the same pattern appears in multiple functions (lines 146, 193, 229). This is a DRY violation at the file level - each function independently fetches the color scheme.

Consider passing colorScheme as a parameter to addHookCommand and removeHookCommand at the function signature level, or better yet, use the helper method pattern suggested in other comments.

The `colorScheme` variable is declared once at line 104, but then the same pattern appears in multiple functions (lines 146, 193, 229). This is a DRY violation at the file level - each function independently fetches the color scheme. Consider passing `colorScheme` as a parameter to `addHookCommand` and `removeHookCommand` at the function signature level, or better yet, use the helper method pattern suggested in other comments.

github-actions[bot] commented

2026-02-12 01:00:28 -06:00

(Migrated from github.com)

PR Summary (auto-generated)

Refactored CLI success messages across multiple commands to use centralized styles.RenderSuccess() and styles.RenderSuccessWithDetails() functions from the internal/cli/styles package
Affected commands include assignee operations, database management, project git linking, setup operations (Claude/OpenCode), and task management (assign/estimate)
Replaced inconsistent fmt.Printf("✓ ...") patterns with standardized rendering that respects the user's color scheme configuration
Added comprehensive golden tests in internal/cli/styles/success_test.go covering all five color schemes (default, monochrome, wave, dragon, lotus) with 273 test cases
All success messages now consistently format output with checkmark character, proper color handling, and newline termination
Updated helper functions in setup commands to accept colorScheme parameter for consistent styling throughout the application

**PR Summary** _(auto-generated)_ - Refactored CLI success messages across multiple commands to use centralized `styles.RenderSuccess()` and `styles.RenderSuccessWithDetails()` functions from the `internal/cli/styles` package - Affected commands include assignee operations, database management, project git linking, setup operations (Claude/OpenCode), and task management (assign/estimate) - Replaced inconsistent `fmt.Printf("✓ ...")` patterns with standardized rendering that respects the user's color scheme configuration - Added comprehensive golden tests in `internal/cli/styles/success_test.go` covering all five color schemes (default, monochrome, wave, dragon, lotus) with 273 test cases - All success messages now consistently format output with checkmark character, proper color handling, and newline termination - Updated helper functions in setup commands to accept `colorScheme` parameter for consistent styling throughout the application

claude[bot] (Migrated from github.com) reviewed

2026-02-12 01:00:30 -06:00

internal/cli/setup/claude.go

					
				@ -202,3 +203,4 @@

					cli.PrintSuccess("Claude hooks removed")

					return nil

				}

The signature now includes colorScheme parameter, but this is being fetched in the calling function. This creates tight coupling and forces every caller to know about color schemes.

Better approach: Have addHookCommand call cli.GetColorScheme() internally if it needs to print, or use a dedicated method on CLI for printing success messages.

The signature now includes `colorScheme` parameter, but this is being fetched in the calling function. This creates tight coupling and forces every caller to know about color schemes. Better approach: Have `addHookCommand` call `cli.GetColorScheme()` internally if it needs to print, or use a dedicated method on CLI for printing success messages.

claude[bot] (Migrated from github.com) reviewed

2026-02-12 01:00:33 -06:00

internal/cli/setup/claude.go

Guard pattern violation: The boolean return value from addHookCommand is used to conditionally print. This creates nested logic.

Refactor to early return pattern:

if !addHookCommand(hooks, "SessionStart", "paso tutorial", colorScheme) {
    return
}
fmt.Print(styles.RenderSuccess("Registered SessionStart hook", colorScheme))

Or better, have addHookCommand handle its own output internally rather than returning a boolean for conditional printing.

Guard pattern violation: The boolean return value from `addHookCommand` is used to conditionally print. This creates nested logic. Refactor to early return pattern: ```go if !addHookCommand(hooks, "SessionStart", "paso tutorial", colorScheme) { return } fmt.Print(styles.RenderSuccess("Registered SessionStart hook", colorScheme)) ``` Or better, have `addHookCommand` handle its own output internally rather than returning a boolean for conditional printing.

claude[bot] (Migrated from github.com) reviewed

2026-02-12 01:00:36 -06:00

internal/cli/db/add.go

					
				@ -96,9 +97,9 @@ func runAdd(cmd *cobra.Command, args []string) error {

					}

Same pattern of fetching colorScheme and calling styles.RenderSuccess with fmt.Sprintf. This exact pattern appears in both branches (lines 102 and 104) and throughout the entire PR.

The duplication extends to:

cli.GetColorScheme() call
fmt.Print() wrapping
styles.RenderSuccess() call
fmt.Sprintf() for string formatting

All of these could be consolidated into a single helper method.

Same pattern of fetching `colorScheme` and calling `styles.RenderSuccess` with `fmt.Sprintf`. This exact pattern appears in both branches (lines 102 and 104) and throughout the entire PR. The duplication extends to: - `cli.GetColorScheme()` call - `fmt.Print()` wrapping - `styles.RenderSuccess()` call - `fmt.Sprintf()` for string formatting All of these could be consolidated into a single helper method.

claude[bot] (Migrated from github.com) reviewed

2026-02-12 01:00:39 -06:00

internal/cli/task/assign.go

					
				@ -132,9 +132,9 @@ func assignTask(cmd *cobra.Command, cliInstance *cli.CLI, taskID int, assigneeID

					}

Another instance of the same repetitive pattern. This file has the exact same structure:

Get color scheme
Conditional logic
Format string with fmt.Sprintf
Call styles.RenderSuccess
Print result

This pattern is repeated across 13 files with ~26 occurrences total in this PR.

Another instance of the same repetitive pattern. This file has the exact same structure: 1. Get color scheme 2. Conditional logic 3. Format string with `fmt.Sprintf` 4. Call `styles.RenderSuccess` 5. Print result This pattern is repeated across 13 files with ~26 occurrences total in this PR.

claude[bot] commented

2026-02-12 01:01:23 -06:00

(Migrated from github.com)

Code Review Summary

This PR successfully standardizes stdout formatting across the codebase with comprehensive test coverage. However, there are significant code quality issues that need to be addressed:

Critical Issues

1. Severe DRY Violation (High Priority)

The pattern of fetching color scheme and rendering success messages is duplicated ~26 times across 13 files:

colorScheme := cli.GetColorScheme()
fmt.Print(styles.RenderSuccess(fmt.Sprintf("...", args), colorScheme))

Impact:

High maintenance burden (any change requires updating 26+ locations)
Increased cognitive load for developers
More surface area for bugs

Recommended Fix:
Create helper methods on the CLI instance:

func (c *CLI) PrintSuccess(message string) {
    fmt.Print(styles.RenderSuccess(message, GetColorScheme()))
}

func (c *CLI) PrintSuccessf(format string, args ...any) {
    c.PrintSuccess(fmt.Sprintf(format, args...))
}

This would reduce each call site to:

cliInstance.PrintSuccessf("Assignee %d deleted successfully", assigneeID)

2. Guard Pattern Violations

Multiple locations have unnecessary nesting that could be flattened:

claude.go:107-113 - Boolean return used for conditional printing instead of having the function handle its own output
opencode.go:98 - Color scheme fetched before early return check, wasting cycles when plugin already installed

3. Tight Coupling in Function Signatures

claude.go:215 and claude.go:261 - Functions now require colorScheme parameter, forcing all callers to know about color schemes. This violates the Single Responsibility Principle - these functions should handle their own presentation concerns.

Test Coverage

The comprehensive golden file test suite (273 lines) is well-structured and provides good coverage across all color schemes.

Files with Issues

Most critical files needing refactoring:

internal/cli/assignee/delete.go
internal/cli/assignee/set.go
internal/cli/db/add.go
internal/cli/db/connect.go
internal/cli/db/remove.go
internal/cli/setup/claude.go
internal/cli/setup/opencode.go
internal/cli/task/assign.go
internal/cli/task/estimate.go
internal/cli/project/git_link.go
internal/cli/project/git_unlink.go

Recommendation

Before merging, extract the duplicated pattern into helper methods. This is a straightforward refactoring that will significantly improve maintainability.

## Code Review Summary This PR successfully standardizes stdout formatting across the codebase with comprehensive test coverage. However, there are significant code quality issues that need to be addressed: ### Critical Issues #### 1. **Severe DRY Violation** (High Priority) The pattern of fetching color scheme and rendering success messages is duplicated **~26 times** across 13 files: ```go colorScheme := cli.GetColorScheme() fmt.Print(styles.RenderSuccess(fmt.Sprintf("...", args), colorScheme)) ``` **Impact:** - High maintenance burden (any change requires updating 26+ locations) - Increased cognitive load for developers - More surface area for bugs **Recommended Fix:** Create helper methods on the CLI instance: ```go func (c *CLI) PrintSuccess(message string) { fmt.Print(styles.RenderSuccess(message, GetColorScheme())) } func (c *CLI) PrintSuccessf(format string, args ...any) { c.PrintSuccess(fmt.Sprintf(format, args...)) } ``` This would reduce each call site to: ```go cliInstance.PrintSuccessf("Assignee %d deleted successfully", assigneeID) ``` #### 2. **Guard Pattern Violations** Multiple locations have unnecessary nesting that could be flattened: **claude.go:107-113** - Boolean return used for conditional printing instead of having the function handle its own output **opencode.go:98** - Color scheme fetched before early return check, wasting cycles when plugin already installed #### 3. **Tight Coupling in Function Signatures** **claude.go:215** and **claude.go:261** - Functions now require `colorScheme` parameter, forcing all callers to know about color schemes. This violates the Single Responsibility Principle - these functions should handle their own presentation concerns. ### Test Coverage The comprehensive golden file test suite (273 lines) is well-structured and provides good coverage across all color schemes. ### Files with Issues Most critical files needing refactoring: - `internal/cli/assignee/delete.go` - `internal/cli/assignee/set.go` - `internal/cli/db/add.go` - `internal/cli/db/connect.go` - `internal/cli/db/remove.go` - `internal/cli/setup/claude.go` - `internal/cli/setup/opencode.go` - `internal/cli/task/assign.go` - `internal/cli/task/estimate.go` - `internal/cli/project/git_link.go` - `internal/cli/project/git_unlink.go` ### Recommendation Before merging, extract the duplicated pattern into helper methods. This is a straightforward refactoring that will significantly improve maintainability.

Sign in to join this conversation.

No reviewers