Revert strconv changes - keep fmt.Sprintf to pass linter#8
Conversation
## Description Reverts the strconv.Itoa changes back to fmt.Sprintf as golangci-lint reports S1039 false positive for these cases. The fmt.Sprintf is needed for proper zero-padding in SQLite strftime queries. ## Type of Change - [x] ♻️ Code refactoring ## Changes Made 1. Reverted holiday_repository.go to use fmt.Sprintf 2. Reverted pkg/client/client.go to use fmt.Sprintf ## How Has This Been Tested? - [x] Manual testing
ilramdhan
added
bug
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| } | ||
| if month != nil { | ||
| q.Add("month", strconv.Itoa(*month)) | ||
| q.Add("month", fmt.Sprintf("%d", *month)) |
There was a problem hiding this comment.
CRITICAL: Month filtering will break - the API handler parses month as integer (1-12), but this sends zero-padded string (01-12). When user passes month=1, this sends "01" which fails the integer parsing in the handler (strconv.Atoi("01") returns 1, but the filter won't match the "01" in SQL).
| if filter.Month != nil { | ||
| whereConditions = append(whereConditions, "strftime('%m', date) = ?") | ||
| args = append(args, strconv.Itoa(*filter.Month)) | ||
| args = append(args, fmt.Sprintf("%02d", *filter.Month)) |
There was a problem hiding this comment.
WARNING: Repository expects month in "%02d" format (e.g., "01", "02"), but the handler sends month as integer (1, 2) to the repository. This will cause month filtering to silently fail.
| if filter.Day != nil { | ||
| whereConditions = append(whereConditions, "strftime('%d', date) = ?") | ||
| args = append(args, strconv.Itoa(*filter.Day)) | ||
| args = append(args, fmt.Sprintf("%02d", *filter.Day)) |
There was a problem hiding this comment.
WARNING: Same issue as line 98 - day filter expects "%02d" format but receives integer.
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Issue DetailsCRITICAL
WARNING
Root CauseThe code changes replaced
Result: The month filter will not match because the repository compares "01" against integer 1. Suggested Fix
Files Reviewed (2 files)
This is a Go (Golang) API project with SQLite database. |
There was a problem hiding this comment.
Pull request overview
This PR reverts integer-to-string conversions from strconv.Itoa back to fmt.Sprintf in a couple of places, motivated by golangci-lint/staticcheck behavior and SQLite strftime zero-padding requirements for month/day filters.
Changes:
- Switched query parameter serialization in
pkg/client/client.goback tofmt.Sprintf. - Switched SQLite
strftimefilter argument formatting ininternal/repository/holiday_repository.goback tofmt.Sprintf(including%02dpadding for month/day).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/client/client.go | Reverts year/month query parameter string conversions to fmt.Sprintf. |
| internal/repository/holiday_repository.go | Reverts date-part filter argument formatting to fmt.Sprintf (with zero-padding for month/day). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if year != nil { | ||
| q.Add("year", strconv.Itoa(*year)) | ||
| q.Add("year", fmt.Sprintf("%d", *year)) | ||
| } | ||
| if month != nil { | ||
| q.Add("month", strconv.Itoa(*month)) | ||
| q.Add("month", fmt.Sprintf("%d", *month)) |
There was a problem hiding this comment.
fmt.Sprintf("%d", ...) on query parameters will be flagged by staticcheck (S1039) as unnecessary formatting. Since there’s no zero-padding needed here, prefer strconv.Itoa(...) (and re-add the strconv import) to keep golangci-lint green.
| if filter.Year != nil { | ||
| whereConditions = append(whereConditions, "strftime('%Y', date) = ?") | ||
| args = append(args, strconv.Itoa(*filter.Year)) | ||
| args = append(args, fmt.Sprintf("%d", *filter.Year)) |
There was a problem hiding this comment.
fmt.Sprintf("%d", *filter.Year) is also likely to trigger staticcheck S1039 (unnecessary Sprintf). Using strconv.Itoa(*filter.Year) avoids the lint issue and would also make the existing strconv import in this file meaningful (right now it’s unused, which will fail the build).
| args = append(args, fmt.Sprintf("%d", *filter.Year)) | |
| args = append(args, strconv.Itoa(*filter.Year)) |
Description
Reverts the strconv.Itoa changes back to fmt.Sprintf as golangci-lint reports S1039 false positive for these cases. The fmt.Sprintf is needed for proper zero-padding in SQLite strftime queries.
Type of Change
Changes Made
How Has This Been Tested?
Description
Fixes # (issue number)
Type of Change
Changes Made
How Has This Been Tested?
Test Configuration:
Checklist
Screenshots (if applicable)
Additional Notes
Related PRs