refactor: reduce code duplication flagged by SonarQube#24
Conversation
Fix 11 SonarQube maintainability issues by deduplicating string literals: - client.go: API path patterns (4 constants) - bulk.go: flag description (1 constant) - override.go: confirmation messages (3 constants) - definition.go: file handling strings (3 constants)
There was a problem hiding this comment.
Pull request overview
Refactors the stackctl CLI to reduce duplicated string literals (SonarQube maintainability) by introducing shared constants for repeated API paths, error messages, and flag descriptions.
Changes:
- Extract repeated API endpoint path strings into constants in the HTTP client.
- Extract repeated confirmation/error message strings and flag descriptions into constants in commands.
- Standardize repeated flag description text (e.g., bulk
--ids) via a shared constant.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| cli/pkg/client/client.go | Introduces constants for frequently used API path templates and reuses them in definition/override-related methods. |
| cli/cmd/override.go | Extracts repeated confirmation error/message strings and the --yes flag description into constants. |
| cli/cmd/definition.go | Extracts repeated --from-file flag name and repeated file/path error strings into constants. |
| cli/cmd/bulk.go | Extracts the repeated --ids flag description into a single constant. |
Fix staticcheck SA1006: fmt.Errorf with dynamic format string and no args should use errors.New; format strings with args stay inline.
|
|
||
| const ( | ||
| flagFromFile = "from-file" | ||
| errPathTraversal = "file path must not contain '..' segments" |
There was a problem hiding this comment.
errPathTraversal is a string constant, but the err* name reads like an error sentinel. Consider either renaming it to msgPathTraversal/errMsgPathTraversal, or changing it to a package-level var error (e.g., var ErrPathTraversal = errors.New(...)) so the name matches the type and callers can use errors.Is.
| errPathTraversal = "file path must not contain '..' segments" | |
| msgPathTraversal = "file path must not contain '..' segments" |
| errReadConfirmation = "reading confirmation: %w" | ||
| msgAborted = "Aborted." | ||
| flagDescSkipConfirm = "Skip confirmation prompt" |
There was a problem hiding this comment.
errReadConfirmation is a format string (it contains %w), not an error value. Renaming it to something like errReadConfirmationFmt (or defining a sentinel error when appropriate) would make its intent/type clearer and prevent accidental misuse (e.g., passing it where an error is expected).
| errReadConfirmation = "reading confirmation: %w" | |
| msgAborted = "Aborted." | |
| flagDescSkipConfirm = "Skip confirmation prompt" | |
| errReadConfirmationFmt = "reading confirmation: %w" | |
| msgAborted = "Aborted." | |
| flagDescSkipConfirm = "Skip confirmation prompt" |
- errPathTraversal → msgPathTraversal (not an error format string) - errReadConfirmation → errReadConfirmationFmt (contains format verb)
- Add confirmAction() to root.go for the confirmation prompt pattern - Add deleteByID() to root.go for single-ID delete commands - Add deleteChartOverride() to override.go for two-ID override deletes - Add readFileErr() to definition.go for file read error wrapping - Replace 8 duplicated confirmation blocks across 4 files - Replace 3 duplicated delete handlers (definition, stack, override×2) - Removes errReadConfirmationFmt constant (now in confirmAction) SonarQube: 0 open issues, quality gate OK, duplication 0.8%
| // confirmAction prompts the user for confirmation unless --yes is set. | ||
| // Returns true if the action should proceed, false if aborted. | ||
| func confirmAction(cmd *cobra.Command, message string) (bool, error) { | ||
| yes, _ := cmd.Flags().GetBool("yes") | ||
| if yes { |
There was a problem hiding this comment.
PR title/description says this change is only about extracting duplicated string literals into constants across 4 files, but this hunk also introduces new shared helper functions (confirmAction/deleteByID) and the PR touches additional files. Please update the PR description (or split the refactor) so reviewers understand the behavioral/structural changes being made.
omattsson
changed the title
Summary
Addresses all 12 SonarQube maintainability issues (duplicated string literals) and reduces overall code duplication from 3.2% to 0.8%.
Changes
Constants for duplicated string literals
pathDefinition,pathOverride,pathBranchOverride,pathQuotaOverride)flagDescIDsconstantmsgAborted,flagDescSkipConfirmconstantsflagFromFile,msgPathTraversalconstants; addreadFileErr()helper for duplicated file-read error wrappingShared helper functions to reduce structural duplication
confirmAction()— extracts the 8-instance confirmation prompt pattern (parse --yes flag, prompt, read stdin, check answer)deleteByID()— extracts the- cli/cmd/root.go: AdddeleteByID()— extracts the- **cli� - **cli/�� output) use- cli/cmd/root.go: AdddeleteByID()— extracts the- cli/cmd/root.go: AdddeleteByID()— extraccts the shared two-ID override del- cli/cmd/root.go: AdddeleteByID()ov- cli/cmd/root.go: AdddeleteByID()— extracts the- cli/cmd/root.go: AdddeleteByID()— extracts the- **cli� - **cli/�� output) use- **cli/cmd/us- cli/cmd/root.go: AdddeleteByID()— extraclp- **cli/cmi/cmd/override.go` — constants + use helperscli/pkg/client/client.g-cli/pkg/client/client.g-cli/pkg/client/client.g-cli/pkg/client/client.g-cli/pkg/client/client.g-cli/pkg/client/client.g- `cli/pkg/client/K** || New code coverage | — | 90.8% |
| New code duplication | — | 0.0% |
| Overall duplication | 3.2% | 0.8% |
Testing
go vetandgo buildclean