Test/estimate coverage #67

Merged
noe.trevino merged 16 commits from test/estimate-coverage into main 2026-02-13 01:07:19 -06:00
Owner

up cli package coverage

up cli package coverage
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:34:26 -06:00
@ -0,0 +1,62 @@
package task
claude[bot] (Migrated from github.com) commented 2026-02-12 22:34:26 -06:00

DRY Violation: String concatenation with fmt.Sprintf in a loop. Consider using strings.Builder for better performance and consistency with other formatting functions in the codebase (see FormatShowHuman at line 128 in show_logic.go).

func FormatBlockedQuiet(result *BlockedResult) string {
	var output strings.Builder
	for _, t := range result.Tasks {
		output.WriteString(fmt.Sprintf("%d\n", t.ID))
	}
	return output.String()
}
**DRY Violation**: String concatenation with `fmt.Sprintf` in a loop. Consider using `strings.Builder` for better performance and consistency with other formatting functions in the codebase (see `FormatShowHuman` at line 128 in show_logic.go). ```suggestion func FormatBlockedQuiet(result *BlockedResult) string { var output strings.Builder for _, t := range result.Tasks { output.WriteString(fmt.Sprintf("%d\n", t.ID)) } return output.String() } ```
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:34:30 -06:00
@ -0,0 +76,4 @@
}
// FormatListQuiet generates quiet output for listing (IDs only)
func FormatListQuiet(result *ListInProgressResult) string {
claude[bot] (Migrated from github.com) commented 2026-02-12 22:34:30 -06:00

DRY Violation: Same string concatenation pattern in loop. Use strings.Builder for consistency.

**DRY Violation**: Same string concatenation pattern in loop. Use `strings.Builder` for consistency.
github-actions[bot] commented 2026-02-12 22:34:32 -06:00 (Migrated from github.com)

PR Summary (auto-generated)

  • Refactored CLI task commands to extract business logic from Cobra command handlers into separate *_logic.go files for improved testability and separation of concerns
  • Added comprehensive unit test coverage across 18 new test files (*_logic_test.go) covering all CLI task operations including assign, blocked, comment, create, delete, done, estimate, in_progress, link, list, move, ready, show, and update commands
  • Introduced structured input/output types (e.g., AssignInput, AssignResult) and parsing functions (e.g., ParseAssignArgs) to replace inline argument parsing and validation
  • Extracted formatting logic into dedicated functions (FormatAssignOutput, FormatAssignJSON) to separate presentation concerns from business logic
  • Affected package: internal/cli/task with 50 files changed (+7,921 additions, -745 deletions), implementing a consistent pattern across all task command implementations
  • Tests use table-driven testing with parallel execution and cover edge cases including invalid inputs, empty values, and error conditions
**PR Summary** _(auto-generated)_ - Refactored CLI task commands to extract business logic from Cobra command handlers into separate *_logic.go files for improved testability and separation of concerns - Added comprehensive unit test coverage across 18 new test files (*_logic_test.go) covering all CLI task operations including assign, blocked, comment, create, delete, done, estimate, in_progress, link, list, move, ready, show, and update commands - Introduced structured input/output types (e.g., AssignInput, AssignResult) and parsing functions (e.g., ParseAssignArgs) to replace inline argument parsing and validation - Extracted formatting logic into dedicated functions (FormatAssignOutput, FormatAssignJSON) to separate presentation concerns from business logic - Affected package: internal/cli/task with 50 files changed (+7,921 additions, -745 deletions), implementing a consistent pattern across all task command implementations - Tests use table-driven testing with parallel execution and cover edge cases including invalid inputs, empty values, and error conditions
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:34:37 -06:00
@ -0,0 +1,133 @@
package task
claude[bot] (Migrated from github.com) commented 2026-02-12 22:34:37 -06:00

Guard Clause Opportunity: Invert the condition to reduce nesting and improve readability.

func FormatListHuman(result *ListInProgressResult) string {
	if result.Count == 0 {
		return "No in-progress tasks found"
	}

	output := fmt.Sprintf("Found %d in-progress tasks:\n\n", result.Count)
	for _, task := range result.Tasks {
		priorityInfo := ""
		if task.PriorityDescription != "" && task.PriorityDescription != "medium" {
			priorityInfo = fmt.Sprintf(" [%s]", task.PriorityDescription)
		}

		blockedInfo := ""
		if task.IsBlocked {
			blockedInfo = " ▲ BLOCKED"
		}

		output += fmt.Sprintf("  [%d] %s%s%s\n", task.ID, task.Title, priorityInfo, blockedInfo)
	}

	return output
}
**Guard Clause Opportunity**: Invert the condition to reduce nesting and improve readability. ```suggestion func FormatListHuman(result *ListInProgressResult) string { if result.Count == 0 { return "No in-progress tasks found" } output := fmt.Sprintf("Found %d in-progress tasks:\n\n", result.Count) for _, task := range result.Tasks { priorityInfo := "" if task.PriorityDescription != "" && task.PriorityDescription != "medium" { priorityInfo = fmt.Sprintf(" [%s]", task.PriorityDescription) } blockedInfo := "" if task.IsBlocked { blockedInfo = " ▲ BLOCKED" } output += fmt.Sprintf(" [%d] %s%s%s\n", task.ID, task.Title, priorityInfo, blockedInfo) } return output } ```
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:34:43 -06:00
@ -0,0 +28,4 @@
// FormatBlockedOutput generates the human-readable output message
func FormatBlockedOutput(result *BlockedResult) string {
if result.Count == 0 {
claude[bot] (Migrated from github.com) commented 2026-02-12 22:34:43 -06:00

Guard Clause Opportunity: Early return flattens the nested code structure.

func FormatBlockedOutput(result *BlockedResult) string {
	if result.Count == 0 {
		return "No blocked tasks found"
	}

	output := fmt.Sprintf("Found %d blocked tasks:\n\n", result.Count)
	for _, t := range result.Tasks {
		priorityInfo := ""
		if t.PriorityDescription != "" && t.PriorityDescription != "medium" {
			priorityInfo = fmt.Sprintf(" [%s]", t.PriorityDescription)
		}
		output += fmt.Sprintf("  [%d] %s%s (BLOCKED)\n", t.ID, t.Title, priorityInfo)
	}
	return output
}
**Guard Clause Opportunity**: Early return flattens the nested code structure. ```suggestion func FormatBlockedOutput(result *BlockedResult) string { if result.Count == 0 { return "No blocked tasks found" } output := fmt.Sprintf("Found %d blocked tasks:\n\n", result.Count) for _, t := range result.Tasks { priorityInfo := "" if t.PriorityDescription != "" && t.PriorityDescription != "medium" { priorityInfo = fmt.Sprintf(" [%s]", t.PriorityDescription) } output += fmt.Sprintf(" [%d] %s%s (BLOCKED)\n", t.ID, t.Title, priorityInfo) } return output } ```
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:34:48 -06:00
@ -0,0 +34,4 @@
output := fmt.Sprintf("Found %d blocked tasks:\n\n", result.Count)
for _, t := range result.Tasks {
priorityInfo := ""
claude[bot] (Migrated from github.com) commented 2026-02-12 22:34:48 -06:00

DRY Violation: The pattern of checking PriorityDescription != "" && PriorityDescription != "medium" appears in multiple files (blocked_logic.go:37, in_progress_logic.go:101). Extract to a shared helper function:

// ShouldDisplayPriority returns true if priority should be shown in output
func ShouldDisplayPriority(priorityDesc string) bool {
	return priorityDesc != "" && priorityDesc != "medium"
}
**DRY Violation**: The pattern of checking `PriorityDescription != "" && PriorityDescription != "medium"` appears in multiple files (blocked_logic.go:37, in_progress_logic.go:101). Extract to a shared helper function: ```go // ShouldDisplayPriority returns true if priority should be shown in output func ShouldDisplayPriority(priorityDesc string) bool { return priorityDesc != "" && priorityDesc != "medium" } ```
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:34:52 -06:00
@ -0,0 +89,4 @@
labelNames[j] = lbl.Name
}
labelsStr := strings.Join(labelNames, ", ")
if labelsStr == "" {
claude[bot] (Migrated from github.com) commented 2026-02-12 22:34:53 -06:00

Guard Clause Opportunity: Apply early return to reduce nesting depth.

		labelsStr := strings.Join(labelNames, ", ")
		if labelsStr == "" {
			labelsStr = "-"
		}
**Guard Clause Opportunity**: Apply early return to reduce nesting depth. ```suggestion labelsStr := strings.Join(labelNames, ", ") if labelsStr == "" { labelsStr = "-" } ```
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:34:55 -06:00
@ -0,0 +36,4 @@
return description, nil
}
if reader == nil {
claude[bot] (Migrated from github.com) commented 2026-02-12 22:34:55 -06:00

Unnecessary nil check: The function signature accepts io.Reader which can be nil, but the nil check happens after attempting to use it. The guard clause at line 39 is redundant since the caller should ensure reader is non-nil when description is "-". Consider documenting this precondition or enforcing it.

**Unnecessary nil check**: The function signature accepts `io.Reader` which can be nil, but the nil check happens after attempting to use it. The guard clause at line 39 is redundant since the caller should ensure reader is non-nil when description is "-". Consider documenting this precondition or enforcing it.
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:34:59 -06:00
@ -0,0 +50,4 @@
// BuildCreateDescription is a convenience function that reads description from a string value
// This matches the pattern in create.go where stdin is handled
func BuildCreateDescription(description string, stdinAvailable bool, reader io.Reader) (string, error) {
claude[bot] (Migrated from github.com) commented 2026-02-12 22:34:59 -06:00

Duplicate Logic: BuildCreateDescription duplicates logic from ReadDescription (lines 34-48). The function adds minimal value - just an extra stdinAvailable check. Consider either:

  1. Removing BuildCreateDescription and handling stdin availability at the call site
  2. Making ReadDescription handle the stdin check internally

This violates DRY principle.

**Duplicate Logic**: `BuildCreateDescription` duplicates logic from `ReadDescription` (lines 34-48). The function adds minimal value - just an extra `stdinAvailable` check. Consider either: 1. Removing `BuildCreateDescription` and handling stdin availability at the call site 2. Making `ReadDescription` handle the stdin check internally This violates DRY principle.
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:35:02 -06:00
@ -0,0 +1,268 @@
package task
claude[bot] (Migrated from github.com) commented 2026-02-12 22:35:02 -06:00

DRY Violation: The pattern of checking for nil/empty and setting a default display value appears repeatedly (lines 170-177, 180-187). Extract to helper:

func displayOrDefault(value *string, defaultVal string) string {
	if value != nil && *value != "" {
		return *value
	}
	return defaultVal
}

Then use:

assigneeDisplay := displayOrDefault(task.AssigneeName, "None")
estimateDisplay := displayOrDefault(task.Estimate, "None")
**DRY Violation**: The pattern of checking for nil/empty and setting a default display value appears repeatedly (lines 170-177, 180-187). Extract to helper: ```go func displayOrDefault(value *string, defaultVal string) string { if value != nil && *value != "" { return *value } return defaultVal } ``` Then use: ```go assigneeDisplay := displayOrDefault(task.AssigneeName, "None") estimateDisplay := displayOrDefault(task.Estimate, "None") ```
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:35:05 -06:00
@ -0,0 +55,4 @@
// validateMoveTarget checks if a move is valid and returns an error message if not
// Returns empty string if valid
func validateMoveTarget(targetType, toColumnName, currentColumnName string) string {
claude[bot] (Migrated from github.com) commented 2026-02-12 22:35:05 -06:00

Guard Clause Opportunity: Invert condition to reduce nesting.

func validateMoveTarget(targetType, toColumnName, currentColumnName string) string {
	if toColumnName != "Unknown" {
		return ""
	}
	
	switch targetType {
	case "next":
		return fmt.Sprintf("task is already in the last column (%s)", currentColumnName)
	case "prev":
		return fmt.Sprintf("task is already in the first column (%s)", currentColumnName)
	}
	return ""
}
**Guard Clause Opportunity**: Invert condition to reduce nesting. ```suggestion func validateMoveTarget(targetType, toColumnName, currentColumnName string) string { if toColumnName != "Unknown" { return "" } switch targetType { case "next": return fmt.Sprintf("task is already in the last column (%s)", currentColumnName) case "prev": return fmt.Sprintf("task is already in the first column (%s)", currentColumnName) } return "" } ```
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:35:08 -06:00
@ -0,0 +95,4 @@
DryRun bool `json:"dry_run,omitempty"`
}
// createMoveResult creates a MoveResult struct
claude[bot] (Migrated from github.com) commented 2026-02-12 22:35:08 -06:00

Unnecessary mutation: Lines 96-98 conditionally set DryRun field, but this can be simplified by always setting it in the struct literal:

func createMoveResult(taskID int, fromColumn, toColumn string, dryRun bool) MoveResult {
	return MoveResult{
		Success:    true,
		TaskID:     taskID,
		FromColumn: fromColumn,
		ToColumn:   toColumn,
		DryRun:     dryRun,
	}
}

The omitempty JSON tag already handles serialization when dryRun is false.

**Unnecessary mutation**: Lines 96-98 conditionally set `DryRun` field, but this can be simplified by always setting it in the struct literal: ```suggestion func createMoveResult(taskID int, fromColumn, toColumn string, dryRun bool) MoveResult { return MoveResult{ Success: true, TaskID: taskID, FromColumn: fromColumn, ToColumn: toColumn, DryRun: dryRun, } } ``` The `omitempty` JSON tag already handles serialization when `dryRun` is false.
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:35:10 -06:00
@ -0,0 +1,177 @@
package task
claude[bot] (Migrated from github.com) commented 2026-02-12 22:35:10 -06:00

DRY Violation: Same nil pointer check and default pattern as in show_logic.go. Use shared helper function (see comment on show_logic.go:170).

**DRY Violation**: Same nil pointer check and default pattern as in `show_logic.go`. Use shared helper function (see comment on show_logic.go:170).
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:35:13 -06:00
@ -0,0 +69,4 @@
}
// HasContentUpdate checks if title or description updates are present
func HasContentUpdate(input *UpdateInput) bool {
claude[bot] (Migrated from github.com) commented 2026-02-12 22:35:13 -06:00

Unnecessary function: HasContentUpdate and HasPriorityUpdate (lines 68-75) are trivial one-liner checks that don't add meaningful abstraction. Direct field checks would be clearer at call sites:

// Instead of: if HasContentUpdate(input)
// Use: if input.Title != nil || input.Description != nil

These functions don't encapsulate complex logic and violate YAGNI principle.

**Unnecessary function**: `HasContentUpdate` and `HasPriorityUpdate` (lines 68-75) are trivial one-liner checks that don't add meaningful abstraction. Direct field checks would be clearer at call sites: ```go // Instead of: if HasContentUpdate(input) // Use: if input.Title != nil || input.Description != nil ``` These functions don't encapsulate complex logic and violate YAGNI principle.
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:35:16 -06:00
@ -0,0 +55,4 @@
}
// IsConfirmationYes checks if the response is a confirmation (y/yes)
func IsConfirmationYes(response string) bool {
claude[bot] (Migrated from github.com) commented 2026-02-12 22:35:16 -06:00

Premature optimization: Creating a separate function for a simple string comparison adds unnecessary indirection. Inline at call site:

// Instead of: if IsConfirmationYes(response)
// Use: normalized := strings.ToLower(strings.TrimSpace(response))
//      if normalized == "y" || normalized == "yes"

This function doesn't justify the abstraction overhead.

**Premature optimization**: Creating a separate function for a simple string comparison adds unnecessary indirection. Inline at call site: ```go // Instead of: if IsConfirmationYes(response) // Use: normalized := strings.ToLower(strings.TrimSpace(response)) // if normalized == "y" || normalized == "yes" ``` This function doesn't justify the abstraction overhead.
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:35:24 -06:00
@ -0,0 +32,4 @@
func ParseShowTaskID(args []string, flagID int) (*ShowInput, error) {
var taskID int
if len(args) > 0 {
claude[bot] (Migrated from github.com) commented 2026-02-12 22:35:24 -06:00

Inconsistent validation: This function uses fmt.Sscanf for parsing while all other logic files use strconv.Atoi (assign_logic.go:28, delete_logic.go:25, estimate_logic.go:28, etc.). Use strconv.Atoi for consistency and clearer error messages:

func ParseShowTaskID(args []string, flagID int) (*ShowInput, error) {
	var taskID int

	if len(args) > 0 {
		var err error
		taskID, err = strconv.Atoi(args[0])
		if err != nil {
			return nil, fmt.Errorf("task ID must be a positive integer")
		}
	} else {
		taskID = flagID
	}

	if taskID <= 0 {
		return nil, fmt.Errorf("task ID must be a positive integer")
	}

	return &ShowInput{TaskID: taskID}, nil
}
**Inconsistent validation**: This function uses `fmt.Sscanf` for parsing while all other logic files use `strconv.Atoi` (assign_logic.go:28, delete_logic.go:25, estimate_logic.go:28, etc.). Use `strconv.Atoi` for consistency and clearer error messages: ```suggestion func ParseShowTaskID(args []string, flagID int) (*ShowInput, error) { var taskID int if len(args) > 0 { var err error taskID, err = strconv.Atoi(args[0]) if err != nil { return nil, fmt.Errorf("task ID must be a positive integer") } } else { taskID = flagID } if taskID <= 0 { return nil, fmt.Errorf("task ID must be a positive integer") } return &ShowInput{TaskID: taskID}, nil } ```
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:35:28 -06:00
@ -0,0 +96,4 @@
// Estimate display
estimateStr := DisplayOrDefault(t.Estimate, "-")
// Blocked indicator
claude[bot] (Migrated from github.com) commented 2026-02-12 22:35:28 -06:00

Empty string literal: Using "" for blocked status (line 100) when not blocked, then checking != "" (line 169) is less clear than using a boolean. Consider:

// In BuildTableRows:
blocked := t.IsBlocked  // bool

// In StyleFunc:
if col == 6 && rows[i].Blocked {  // direct bool check

Or if string is needed for table rendering, use constant:

const blockedIndicator = "BLOCKED"
blockedStr := ""
if t.IsBlocked {
    blockedStr = blockedIndicator
}
**Empty string literal**: Using `""` for blocked status (line 100) when not blocked, then checking `!= ""` (line 169) is less clear than using a boolean. Consider: ```go // In BuildTableRows: blocked := t.IsBlocked // bool // In StyleFunc: if col == 6 && rows[i].Blocked { // direct bool check ``` Or if string is needed for table rendering, use constant: ```go const blockedIndicator = "BLOCKED" blockedStr := "" if t.IsBlocked { blockedStr = blockedIndicator } ```
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:35:31 -06:00
@ -0,0 +13,4 @@
)
// findNextColumnName finds the name of the next column in the linked list
func findNextColumnName(columns []*models.Column, currentColumnID int) string {
claude[bot] (Migrated from github.com) commented 2026-02-12 22:35:31 -06:00

Magic string "Unknown": The string literal "Unknown" is used as a sentinel value (lines 20, 34, 54). This violates the Replace Magic Number with Symbolic Constant refactoring. Define as a package constant:

const unknownColumnName = "Unknown"
**Magic string "Unknown"**: The string literal "Unknown" is used as a sentinel value (lines 20, 34, 54). This violates the Replace Magic Number with Symbolic Constant refactoring. Define as a package constant: ```go const unknownColumnName = "Unknown" ```
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:35:33 -06:00
@ -0,0 +40,4 @@
return unknownColumnName
}
// parseMoveTarget determines the type of move target (next, prev, or column name)
claude[bot] (Migrated from github.com) commented 2026-02-12 22:35:34 -06:00

DRY Violation: parseMoveTarget switch cases duplicate string literals ("next", "prev") that appear in validation messages (lines 56, 58). Define as constants:

const (
	moveTargetNext = "next"
	moveTargetPrev = "prev"
)
**DRY Violation**: `parseMoveTarget` switch cases duplicate string literals ("next", "prev") that appear in validation messages (lines 56, 58). Define as constants: ```go const ( moveTargetNext = "next" moveTargetPrev = "prev" ) ```
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:35:38 -06:00
@ -0,0 +20,4 @@
}
// ParseEstimateArgs parses task ID and estimate from command arguments
func ParseEstimateArgs(args []string, clearFlag bool) (*EstimateInput, error) {
claude[bot] (Migrated from github.com) commented 2026-02-12 22:35:38 -06:00

Parameter shadowing: The clearFlag parameter duplicates information already in the returned struct's Clear field. The flag value could be set directly in the struct without the parameter:

func ParseEstimateArgs(args []string, clear bool) (*EstimateInput, error) {
	// ... parsing logic ...
	
	return &EstimateInput{
		TaskID:   taskID,
		Estimate: estimate,
		Clear:    clear,  // directly use the parameter
	}, nil
}

Current implementation is fine, but the naming could be clearer (rename param to clear not clearFlag).

**Parameter shadowing**: The `clearFlag` parameter duplicates information already in the returned struct's `Clear` field. The flag value could be set directly in the struct without the parameter: ```go func ParseEstimateArgs(args []string, clear bool) (*EstimateInput, error) { // ... parsing logic ... return &EstimateInput{ TaskID: taskID, Estimate: estimate, Clear: clear, // directly use the parameter }, nil } ``` Current implementation is fine, but the naming could be clearer (rename param to `clear` not `clearFlag`).
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:35:40 -06:00
@ -0,0 +108,4 @@
blockedInfo := ""
if task.IsBlocked {
blockedInfo = " ▲ BLOCKED"
claude[bot] (Migrated from github.com) commented 2026-02-12 22:35:40 -06:00

DRY Violation: Hardcoded "▲ BLOCKED" indicator. This specific Unicode character and text appears only here. If this is a standard blocked indicator across the app, extract to a constant in a shared package for consistency.

**DRY Violation**: Hardcoded "▲ BLOCKED" indicator. This specific Unicode character and text appears only here. If this is a standard blocked indicator across the app, extract to a constant in a shared package for consistency.
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:35:50 -06:00
@ -104,3 +101,3 @@
assigneeID := &assignee.ID
return assignTask(cmd, cliInstance, taskID, assigneeID, assigneeName, formatter, jsonOutput, quietMode)
return assignTask(cmd, cliInstance, input.TaskID, assigneeID, assigneeName, formatter, jsonOutput, quietMode)
}
claude[bot] (Migrated from github.com) commented 2026-02-12 22:35:50 -06:00

Unclear control flow: The function returns early when clearAssignee is true (line 81), but continues to resolve assignee name and call the same assignTask function. This obscures the fact that both paths converge to the same function. Consider consolidating:

var assigneeID *int
var assigneeName string

if !clearAssignee {
	assigneeName = input.AssigneeName
	if assigneeName == "" {
		// ... resolve from config/user
	}
	assignee, err := cliInstance.App.AssigneeService.GetOrCreate(ctx, assigneeName)
	if err != nil {
		return formatter.Error(cli.ExitError, "ASSIGNEE_ERROR", fmt.Sprintf("failed to resolve assignee '%s': %s", assigneeName, err))
	}
	assigneeID = &assignee.ID
}

return assignTask(cmd, cliInstance, input.TaskID, assigneeID, assigneeName, formatter, jsonOutput, quietMode)
**Unclear control flow**: The function returns early when `clearAssignee` is true (line 81), but continues to resolve assignee name and call the same `assignTask` function. This obscures the fact that both paths converge to the same function. Consider consolidating: ```go var assigneeID *int var assigneeName string if !clearAssignee { assigneeName = input.AssigneeName if assigneeName == "" { // ... resolve from config/user } assignee, err := cliInstance.App.AssigneeService.GetOrCreate(ctx, assigneeName) if err != nil { return formatter.Error(cli.ExitError, "ASSIGNEE_ERROR", fmt.Sprintf("failed to resolve assignee '%s': %s", assigneeName, err)) } assigneeID = &assignee.ID } return assignTask(cmd, cliInstance, input.TaskID, assigneeID, assigneeName, formatter, jsonOutput, quietMode) ```
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:35:52 -06:00
@ -0,0 +33,4 @@
}
// ValidateCommentMessage validates the comment message length
func ValidateCommentMessage(message string) error {
claude[bot] (Migrated from github.com) commented 2026-02-12 22:35:52 -06:00

Magic number: The validation limit 1000 is hardcoded. Extract to a named constant:

const maxCommentLength = 1000

func ValidateCommentMessage(message string) error {
	if len(message) > maxCommentLength {
		return fmt.Errorf("message exceeds %d character limit", maxCommentLength)
	}
	return nil
}
**Magic number**: The validation limit `1000` is hardcoded. Extract to a named constant: ```go const maxCommentLength = 1000 func ValidateCommentMessage(message string) error { if len(message) > maxCommentLength { return fmt.Errorf("message exceeds %d character limit", maxCommentLength) } return nil } ```
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:35:53 -06:00
@ -0,0 +1,77 @@
package task
claude[bot] (Migrated from github.com) commented 2026-02-12 22:35:53 -06:00

Magic number: Truncation length 60 is hardcoded. Extract to constant or make it a parameter if different contexts need different lengths.

**Magic number**: Truncation length `60` is hardcoded. Extract to constant or make it a parameter if different contexts need different lengths.
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:35:57 -06:00
@ -0,0 +59,4 @@
}
// FormatTasksQuiet formats tasks for quiet output (IDs only)
func FormatTasksQuiet(tasks []*models.TaskSummary) []string {
claude[bot] (Migrated from github.com) commented 2026-02-12 22:35:57 -06:00

Unnecessary allocation: Pre-allocating slice with exact size when you know the length:

func FormatTasksQuiet(tasks []*models.TaskSummary) []string {
	if len(tasks) == 0 {
		return nil
	}
	ids := make([]string, len(tasks))
	for i, t := range tasks {
		ids[i] = strconv.Itoa(t.ID)
	}
	return ids
}

Though minor, returning nil for empty input is more idiomatic than returning an empty slice with zero capacity.

**Unnecessary allocation**: Pre-allocating slice with exact size when you know the length: ```suggestion func FormatTasksQuiet(tasks []*models.TaskSummary) []string { if len(tasks) == 0 { return nil } ids := make([]string, len(tasks)) for i, t := range tasks { ids[i] = strconv.Itoa(t.ID) } return ids } ``` Though minor, returning `nil` for empty input is more idiomatic than returning an empty slice with zero capacity.
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:36:01 -06:00
@ -0,0 +70,4 @@
}
// formatMoveMessage generates the human-readable message for a move operation
func formatMoveMessage(taskID int, fromColumn, toColumn string, dryRun bool) string {
claude[bot] (Migrated from github.com) commented 2026-02-12 22:36:01 -06:00

Complex conditional: The double condition fromColumn == toColumn appears in both dry-run and non-dry-run cases. Extract the same-column check to eliminate duplication:

func formatMoveMessage(taskID int, fromColumn, toColumn string, dryRun bool) string {
	isSameColumn := fromColumn == toColumn
	
	if dryRun {
		if isSameColumn {
			return fmt.Sprintf("Would keep task %d in '%s' (already there)", taskID, toColumn)
		}
		return fmt.Sprintf("Would move task %d from '%s' to '%s'", taskID, fromColumn, toColumn)
	}
	
	if isSameColumn {
		return fmt.Sprintf("Task %d is already in '%s'", taskID, toColumn)
	}
	return fmt.Sprintf("Task %d moved to '%s'", taskID, toColumn)
}
**Complex conditional**: The double condition `fromColumn == toColumn` appears in both dry-run and non-dry-run cases. Extract the same-column check to eliminate duplication: ```suggestion func formatMoveMessage(taskID int, fromColumn, toColumn string, dryRun bool) string { isSameColumn := fromColumn == toColumn if dryRun { if isSameColumn { return fmt.Sprintf("Would keep task %d in '%s' (already there)", taskID, toColumn) } return fmt.Sprintf("Would move task %d from '%s' to '%s'", taskID, fromColumn, toColumn) } if isSameColumn { return fmt.Sprintf("Task %d is already in '%s'", taskID, toColumn) } return fmt.Sprintf("Task %d moved to '%s'", taskID, toColumn) } ```
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:36:03 -06:00
@ -0,0 +1,268 @@
package task
claude[bot] (Migrated from github.com) commented 2026-02-12 22:36:03 -06:00

DRY Violation: Manual line-by-line indentation pattern appears twice (lines 148-150 and 270-272). Extract to helper:

func writeIndentedLines(builder *strings.Builder, text string, indent string, style lipgloss.Style) {
	for _, line := range strings.Split(text, "\n") {
		builder.WriteString(indent + style.Render(line) + "\n")
	}
}
**DRY Violation**: Manual line-by-line indentation pattern appears twice (lines 148-150 and 270-272). Extract to helper: ```go func writeIndentedLines(builder *strings.Builder, text string, indent string, style lipgloss.Style) { for _, line := range strings.Split(text, "\n") { builder.WriteString(indent + style.Render(line) + "\n") } } ```
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:36:06 -06:00
@ -0,0 +252,4 @@
header := fmt.Sprintf("Comments (%d)", len(task.Comments))
content.WriteString(styles.SectionStyle.Render(header))
content.WriteString("\n")
for _, comment := range task.Comments {
claude[bot] (Migrated from github.com) commented 2026-02-12 22:36:06 -06:00

Formatting inconsistency: Comment metadata formatting (lines 266-272) manually constructs indented output while the rest of the function uses the styles package helpers. Consider extracting a RenderComment function in the styles package for consistency.

**Formatting inconsistency**: Comment metadata formatting (lines 266-272) manually constructs indented output while the rest of the function uses the `styles` package helpers. Consider extracting a `RenderComment` function in the styles package for consistency.
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:36:09 -06:00
@ -0,0 +45,4 @@
ToColumn string
}
// ParseInProgressArgs parses the command arguments and flags to determine the mode and inputs
claude[bot] (Migrated from github.com) commented 2026-02-12 22:36:09 -06:00

Potential bug: The error message at line 59 states "either provide a task ID or use --project flag" but this is misleading when projectID > 0 is already checked first (line 50). If we reach line 59, it means projectID is NOT provided, so the message should be:

	if len(args) == 0 {
		return nil, fmt.Errorf("task ID is required (or use --project flag to list tasks)")
	}
**Potential bug**: The error message at line 59 states "either provide a task ID or use --project flag" but this is misleading when `projectID > 0` is already checked first (line 50). If we reach line 59, it means projectID is NOT provided, so the message should be: ```suggestion if len(args) == 0 { return nil, fmt.Errorf("task ID is required (or use --project flag to list tasks)") } ```
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:36:11 -06:00
@ -0,0 +149,4 @@
t := table.New().
Border(lipgloss.RoundedBorder()).
BorderStyle(lipgloss.NewStyle().Foreground(lipgloss.Color("238"))).
claude[bot] (Migrated from github.com) commented 2026-02-12 22:36:11 -06:00

Magic color value: Hardcoded color "238" for border style. Extract to constant or use from color scheme parameter for consistency with the rest of the styling.

**Magic color value**: Hardcoded color `"238"` for border style. Extract to constant or use from color scheme parameter for consistency with the rest of the styling.
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:36:24 -06:00
@ -0,0 +1,180 @@
package task
claude[bot] (Migrated from github.com) commented 2026-02-12 22:36:24 -06:00

Missing validation: The test accepts negative task ID (-5) without error, but this is likely a bug in the implementation. Task IDs should typically be positive integers. Add validation in ParseAssignArgs:

if taskID <= 0 {
    return nil, fmt.Errorf("task ID must be a positive integer")
}

This test should expect an error instead of accepting the negative value.

**Missing validation**: The test accepts negative task ID (-5) without error, but this is likely a bug in the implementation. Task IDs should typically be positive integers. Add validation in `ParseAssignArgs`: ```go if taskID <= 0 { return nil, fmt.Errorf("task ID must be a positive integer") } ``` This test should expect an error instead of accepting the negative value.
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:36:27 -06:00
@ -0,0 +27,4 @@
{ID: 4, Title: "Task 4", IsBlocked: false},
},
},
expected: []*models.TaskSummary{
claude[bot] (Migrated from github.com) commented 2026-02-12 22:36:27 -06:00

Test flakiness: The test "filters blocked tasks from multiple columns" (lines 19-34) asserts on slice ordering from a map iteration. Since Go maps have undefined iteration order, the result order is non-deterministic. The test may fail intermittently.

Sort results before comparison or use assert.ElementsMatch instead of direct equality:

assert.ElementsMatch(t, tt.expected, result)
**Test flakiness**: The test "filters blocked tasks from multiple columns" (lines 19-34) asserts on slice ordering from a map iteration. Since Go maps have undefined iteration order, the result order is non-deterministic. The test may fail intermittently. Sort results before comparison or use `assert.ElementsMatch` instead of direct equality: ```go assert.ElementsMatch(t, tt.expected, result) ```
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:36:39 -06:00
@ -0,0 +43,4 @@
}
// FormatDoneJSON generates the JSON output structure
func FormatDoneJSON(result *DoneResult) map[string]any {
claude[bot] (Migrated from github.com) commented 2026-02-12 22:36:40 -06:00

DRY Violation: FormatDoneJSON has identical structure to FormatReadyMoveJSON (ready_move_logic.go:43-50) and nearly identical to FormatMoveJSON (in_progress_logic.go:123-130). Extract to a shared function:

// In a shared utilities file:
func FormatMoveResultJSON(taskID int, fromColumn, toColumn string) map[string]any {
	return map[string]any{
		"success":     true,
		"task_id":     taskID,
		"from_column": fromColumn,
		"to_column":   toColumn,
	}
}
**DRY Violation**: `FormatDoneJSON` has identical structure to `FormatReadyMoveJSON` (ready_move_logic.go:43-50) and nearly identical to `FormatMoveJSON` (in_progress_logic.go:123-130). Extract to a shared function: ```go // In a shared utilities file: func FormatMoveResultJSON(taskID int, fromColumn, toColumn string) map[string]any { return map[string]any{ "success": true, "task_id": taskID, "from_column": fromColumn, "to_column": toColumn, } } ```
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:36:41 -06:00
@ -0,0 +1,50 @@
package task
claude[bot] (Migrated from github.com) commented 2026-02-12 22:36:41 -06:00

DRY Violation: String concatenation in loop - same issue as other quiet formatters. Use strings.Builder.

**DRY Violation**: String concatenation in loop - same issue as other quiet formatters. Use `strings.Builder`.
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:36:42 -06:00
@ -0,0 +21,4 @@
output := fmt.Sprintf("Found %d ready tasks:\n\n", result.Count)
for _, t := range result.Tasks {
priorityInfo := ""
claude[bot] (Migrated from github.com) commented 2026-02-12 22:36:43 -06:00

DRY Violation: Same priority check pattern as in blocked_logic.go:37 and in_progress_logic.go:101. Extract to shared helper.

**DRY Violation**: Same priority check pattern as in blocked_logic.go:37 and in_progress_logic.go:101. Extract to shared helper.
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:36:45 -06:00
@ -0,0 +1,53 @@
package task
claude[bot] (Migrated from github.com) commented 2026-02-12 22:36:46 -06:00

Inconsistent formatting: All other quiet formatters append \n to the ID (e.g., blocked_logic.go:58, in_progress_logic.go:77, ready_logic.go:46), but this one doesn't. This inconsistency could break scripting that relies on consistent output format:

func FormatReadyMoveQuiet(result *ReadyMoveResult) string {
	return fmt.Sprintf("%d\n", result.TaskID)
}
**Inconsistent formatting**: All other quiet formatters append `\n` to the ID (e.g., blocked_logic.go:58, in_progress_logic.go:77, ready_logic.go:46), but this one doesn't. This inconsistency could break scripting that relies on consistent output format: ```suggestion func FormatReadyMoveQuiet(result *ReadyMoveResult) string { return fmt.Sprintf("%d\n", result.TaskID) } ```
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:36:48 -06:00
@ -0,0 +1,53 @@
package task
claude[bot] (Migrated from github.com) commented 2026-02-12 22:36:48 -06:00

Unused field: The AlreadyReady field in ReadyMoveResult struct is never used in any of the formatting functions. Either use it or remove it:

  • If task is already ready, include in output message
  • Or remove the field if not needed
**Unused field**: The `AlreadyReady` field in `ReadyMoveResult` struct is never used in any of the formatting functions. Either use it or remove it: - If task is already ready, include in output message - Or remove the field if not needed
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:36:51 -06:00
@ -0,0 +33,4 @@
}
// DetermineRelationType determines the relation type ID and name based on flags
func DetermineRelationType(blocker, related bool) (int, string) {
claude[bot] (Migrated from github.com) commented 2026-02-12 22:36:51 -06:00

Magic numbers: Relation type IDs (1, 2, 3) are hardcoded in multiple places (lines 32, 34, 36, 42, 43, 45). Define as constants:

const (
	RelationTypeParentChild = 1
	RelationTypeBlocking    = 2
	RelationTypeRelated     = 3
)

Then use in both DetermineRelationType and FormatLinkOutput.

**Magic numbers**: Relation type IDs (1, 2, 3) are hardcoded in multiple places (lines 32, 34, 36, 42, 43, 45). Define as constants: ```go const ( RelationTypeParentChild = 1 RelationTypeBlocking = 2 RelationTypeRelated = 3 ) ``` Then use in both `DetermineRelationType` and `FormatLinkOutput`.
claude[bot] (Migrated from github.com) reviewed 2026-02-12 22:36:54 -06:00
@ -0,0 +33,4 @@
}
// DetermineRelationType determines the relation type ID and name based on flags
func DetermineRelationType(blocker, related bool) (int, string) {
claude[bot] (Migrated from github.com) commented 2026-02-12 22:36:54 -06:00

Code smell - Feature Envy: DetermineRelationType returns both ID and name, which are then stored in separate struct fields. This creates tight coupling between the relation type logic and the result struct. Consider using a RelationType struct or enum:

type RelationType struct {
	ID   int
	Name string
}

var (
	RelationParentChild = RelationType{ID: 1, Name: "parent-child"}
	RelationBlocking    = RelationType{ID: 2, Name: "blocking"}
	RelationRelated     = RelationType{ID: 3, Name: "related"}
)

func DetermineRelationType(blocker, related bool) RelationType {
	if blocker {
		return RelationBlocking
	}
	if related {
		return RelationRelated
	}
	return RelationParentChild
}
**Code smell - Feature Envy**: `DetermineRelationType` returns both ID and name, which are then stored in separate struct fields. This creates tight coupling between the relation type logic and the result struct. Consider using a RelationType struct or enum: ```go type RelationType struct { ID int Name string } var ( RelationParentChild = RelationType{ID: 1, Name: "parent-child"} RelationBlocking = RelationType{ID: 2, Name: "blocking"} RelationRelated = RelationType{ID: 3, Name: "related"} ) func DetermineRelationType(blocker, related bool) RelationType { if blocker { return RelationBlocking } if related { return RelationRelated } return RelationParentChild } ```
claude[bot] commented 2026-02-12 22:37:36 -06:00 (Migrated from github.com)

Code Review Summary

I've completed a detailed review of PR #67 focusing on code quality and refactoring best practices.

Key Issues Found

DRY Violations (Most Critical):

  • Priority display check pattern duplicated across 4+ files
  • Nil/empty pointer checks with defaults repeated in multiple locations
  • String concatenation in loops - should use strings.Builder
  • Identical JSON formatting functions for move operations across 3 files

Guard Clause Opportunities:

  • Multiple functions use nested if-else when early returns would flatten the code

Magic Values:

  • Hardcoded relation type IDs (1, 2, 3) in link_logic.go
  • Unknown sentinel string in move_logic.go
  • Comment length limit (1000) and truncation length (60)

Potential Bugs:

  • Test flakiness: map iteration order in TestFilterBlockedTasks
  • Missing validation: negative task IDs accepted without error
  • Inconsistent output: FormatReadyMoveQuiet missing trailing newline
  • Unused field: AlreadyReady in ReadyMoveResult

Over-Abstraction:

  • Trivial helper functions that don't justify abstraction overhead

All issues have been documented as inline comments on specific lines.

## Code Review Summary I've completed a detailed review of PR #67 focusing on code quality and refactoring best practices. ### Key Issues Found **DRY Violations (Most Critical):** - Priority display check pattern duplicated across 4+ files - Nil/empty pointer checks with defaults repeated in multiple locations - String concatenation in loops - should use strings.Builder - Identical JSON formatting functions for move operations across 3 files **Guard Clause Opportunities:** - Multiple functions use nested if-else when early returns would flatten the code **Magic Values:** - Hardcoded relation type IDs (1, 2, 3) in link_logic.go - Unknown sentinel string in move_logic.go - Comment length limit (1000) and truncation length (60) **Potential Bugs:** - Test flakiness: map iteration order in TestFilterBlockedTasks - Missing validation: negative task IDs accepted without error - Inconsistent output: FormatReadyMoveQuiet missing trailing newline - Unused field: AlreadyReady in ReadyMoveResult **Over-Abstraction:** - Trivial helper functions that don't justify abstraction overhead All issues have been documented as inline comments on specific lines.
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
noe.trevino/paso!67
No description provided.