Test/estimate coverage #67
No reviewers
Labels
No labels
bug
ci
documentation
duplicate
enhancement
feature
good first issue
help wanted
invalid
performance
priority
P0
priority
P1
priority
P2
question
refactor
size
L
size
M
size
S
size
XL
size
XS
tests
trivial
wontfix
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
noe.trevino/paso!67
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "test/estimate-coverage"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
up cli package coverage
@ -0,0 +1,62 @@package taskDRY Violation: String concatenation with
fmt.Sprintfin a loop. Consider usingstrings.Builderfor better performance and consistency with other formatting functions in the codebase (seeFormatShowHumanat line 128 in show_logic.go).@ -0,0 +76,4 @@}// FormatListQuiet generates quiet output for listing (IDs only)func FormatListQuiet(result *ListInProgressResult) string {DRY Violation: Same string concatenation pattern in loop. Use
strings.Builderfor consistency.PR Summary (auto-generated)
@ -0,0 +1,133 @@package taskGuard Clause Opportunity: Invert the condition to reduce nesting and improve readability.
@ -0,0 +28,4 @@// FormatBlockedOutput generates the human-readable output messagefunc FormatBlockedOutput(result *BlockedResult) string {if result.Count == 0 {Guard Clause Opportunity: Early return flattens the nested code structure.
@ -0,0 +34,4 @@output := fmt.Sprintf("Found %d blocked tasks:\n\n", result.Count)for _, t := range result.Tasks {priorityInfo := ""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:@ -0,0 +89,4 @@labelNames[j] = lbl.Name}labelsStr := strings.Join(labelNames, ", ")if labelsStr == "" {Guard Clause Opportunity: Apply early return to reduce nesting depth.
@ -0,0 +36,4 @@return description, nil}if reader == nil {Unnecessary nil check: The function signature accepts
io.Readerwhich 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.@ -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 handledfunc BuildCreateDescription(description string, stdinAvailable bool, reader io.Reader) (string, error) {Duplicate Logic:
BuildCreateDescriptionduplicates logic fromReadDescription(lines 34-48). The function adds minimal value - just an extrastdinAvailablecheck. Consider either:BuildCreateDescriptionand handling stdin availability at the call siteReadDescriptionhandle the stdin check internallyThis violates DRY principle.
@ -0,0 +1,268 @@package taskDRY Violation: The pattern of checking for nil/empty and setting a default display value appears repeatedly (lines 170-177, 180-187). Extract to helper:
Then use:
@ -0,0 +55,4 @@// validateMoveTarget checks if a move is valid and returns an error message if not// Returns empty string if validfunc validateMoveTarget(targetType, toColumnName, currentColumnName string) string {Guard Clause Opportunity: Invert condition to reduce nesting.
@ -0,0 +95,4 @@DryRun bool `json:"dry_run,omitempty"`}// createMoveResult creates a MoveResult structUnnecessary mutation: Lines 96-98 conditionally set
DryRunfield, but this can be simplified by always setting it in the struct literal:The
omitemptyJSON tag already handles serialization whendryRunis false.@ -0,0 +1,177 @@package taskDRY Violation: Same nil pointer check and default pattern as in
show_logic.go. Use shared helper function (see comment on show_logic.go:170).@ -0,0 +69,4 @@}// HasContentUpdate checks if title or description updates are presentfunc HasContentUpdate(input *UpdateInput) bool {Unnecessary function:
HasContentUpdateandHasPriorityUpdate(lines 68-75) are trivial one-liner checks that don't add meaningful abstraction. Direct field checks would be clearer at call sites:These functions don't encapsulate complex logic and violate YAGNI principle.
@ -0,0 +55,4 @@}// IsConfirmationYes checks if the response is a confirmation (y/yes)func IsConfirmationYes(response string) bool {Premature optimization: Creating a separate function for a simple string comparison adds unnecessary indirection. Inline at call site:
This function doesn't justify the abstraction overhead.
@ -0,0 +32,4 @@func ParseShowTaskID(args []string, flagID int) (*ShowInput, error) {var taskID intif len(args) > 0 {Inconsistent validation: This function uses
fmt.Sscanffor parsing while all other logic files usestrconv.Atoi(assign_logic.go:28, delete_logic.go:25, estimate_logic.go:28, etc.). Usestrconv.Atoifor consistency and clearer error messages:@ -0,0 +96,4 @@// Estimate displayestimateStr := DisplayOrDefault(t.Estimate, "-")// Blocked indicatorEmpty string literal: Using
""for blocked status (line 100) when not blocked, then checking!= ""(line 169) is less clear than using a boolean. Consider:Or if string is needed for table rendering, use constant:
@ -0,0 +13,4 @@)// findNextColumnName finds the name of the next column in the linked listfunc findNextColumnName(columns []*models.Column, currentColumnID int) string {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:
@ -0,0 +40,4 @@return unknownColumnName}// parseMoveTarget determines the type of move target (next, prev, or column name)DRY Violation:
parseMoveTargetswitch cases duplicate string literals ("next", "prev") that appear in validation messages (lines 56, 58). Define as constants:@ -0,0 +20,4 @@}// ParseEstimateArgs parses task ID and estimate from command argumentsfunc ParseEstimateArgs(args []string, clearFlag bool) (*EstimateInput, error) {Parameter shadowing: The
clearFlagparameter duplicates information already in the returned struct'sClearfield. The flag value could be set directly in the struct without the parameter:Current implementation is fine, but the naming could be clearer (rename param to
clearnotclearFlag).@ -0,0 +108,4 @@blockedInfo := ""if task.IsBlocked {blockedInfo = " ▲ BLOCKED"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.
@ -104,3 +101,3 @@assigneeID := &assignee.IDreturn assignTask(cmd, cliInstance, taskID, assigneeID, assigneeName, formatter, jsonOutput, quietMode)return assignTask(cmd, cliInstance, input.TaskID, assigneeID, assigneeName, formatter, jsonOutput, quietMode)}Unclear control flow: The function returns early when
clearAssigneeis true (line 81), but continues to resolve assignee name and call the sameassignTaskfunction. This obscures the fact that both paths converge to the same function. Consider consolidating:@ -0,0 +33,4 @@}// ValidateCommentMessage validates the comment message lengthfunc ValidateCommentMessage(message string) error {Magic number: The validation limit
1000is hardcoded. Extract to a named constant:@ -0,0 +1,77 @@package taskMagic number: Truncation length
60is hardcoded. Extract to constant or make it a parameter if different contexts need different lengths.@ -0,0 +59,4 @@}// FormatTasksQuiet formats tasks for quiet output (IDs only)func FormatTasksQuiet(tasks []*models.TaskSummary) []string {Unnecessary allocation: Pre-allocating slice with exact size when you know the length:
Though minor, returning
nilfor empty input is more idiomatic than returning an empty slice with zero capacity.@ -0,0 +70,4 @@}// formatMoveMessage generates the human-readable message for a move operationfunc formatMoveMessage(taskID int, fromColumn, toColumn string, dryRun bool) string {Complex conditional: The double condition
fromColumn == toColumnappears in both dry-run and non-dry-run cases. Extract the same-column check to eliminate duplication:@ -0,0 +1,268 @@package taskDRY Violation: Manual line-by-line indentation pattern appears twice (lines 148-150 and 270-272). Extract to helper:
@ -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 {Formatting inconsistency: Comment metadata formatting (lines 266-272) manually constructs indented output while the rest of the function uses the
stylespackage helpers. Consider extracting aRenderCommentfunction in the styles package for consistency.@ -0,0 +45,4 @@ToColumn string}// ParseInProgressArgs parses the command arguments and flags to determine the mode and inputsPotential bug: The error message at line 59 states "either provide a task ID or use --project flag" but this is misleading when
projectID > 0is already checked first (line 50). If we reach line 59, it means projectID is NOT provided, so the message should be:@ -0,0 +149,4 @@t := table.New().Border(lipgloss.RoundedBorder()).BorderStyle(lipgloss.NewStyle().Foreground(lipgloss.Color("238"))).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.@ -0,0 +1,180 @@package taskMissing 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:This test should expect an error instead of accepting the negative value.
@ -0,0 +27,4 @@{ID: 4, Title: "Task 4", IsBlocked: false},},},expected: []*models.TaskSummary{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.ElementsMatchinstead of direct equality:@ -0,0 +43,4 @@}// FormatDoneJSON generates the JSON output structurefunc FormatDoneJSON(result *DoneResult) map[string]any {DRY Violation:
FormatDoneJSONhas identical structure toFormatReadyMoveJSON(ready_move_logic.go:43-50) and nearly identical toFormatMoveJSON(in_progress_logic.go:123-130). Extract to a shared function:@ -0,0 +1,50 @@package taskDRY Violation: String concatenation in loop - same issue as other quiet formatters. Use
strings.Builder.@ -0,0 +21,4 @@output := fmt.Sprintf("Found %d ready tasks:\n\n", result.Count)for _, t := range result.Tasks {priorityInfo := ""DRY Violation: Same priority check pattern as in blocked_logic.go:37 and in_progress_logic.go:101. Extract to shared helper.
@ -0,0 +1,53 @@package taskInconsistent formatting: All other quiet formatters append
\nto 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:@ -0,0 +1,53 @@package taskUnused field: The
AlreadyReadyfield inReadyMoveResultstruct is never used in any of the formatting functions. Either use it or remove it:@ -0,0 +33,4 @@}// DetermineRelationType determines the relation type ID and name based on flagsfunc DetermineRelationType(blocker, related bool) (int, string) {Magic numbers: Relation type IDs (1, 2, 3) are hardcoded in multiple places (lines 32, 34, 36, 42, 43, 45). Define as constants:
Then use in both
DetermineRelationTypeandFormatLinkOutput.@ -0,0 +33,4 @@}// DetermineRelationType determines the relation type ID and name based on flagsfunc DetermineRelationType(blocker, related bool) (int, string) {Code smell - Feature Envy:
DetermineRelationTypereturns 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: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):
Guard Clause Opportunities:
Magic Values:
Potential Bugs:
Over-Abstraction:
All issues have been documented as inline comments on specific lines.