Update deployment docs and refactor string formatting to strconv#7
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
## Description Updates README.md with Railway deprecation note and fixes deployment links. Refactors string formatting from fmt.Sprintf to strconv.Itoa for consistency. ## Type of Change - [ ] 🐛 Bug fix - [ ] ✨ New feature - [ ] 💥 Breaking change - [x] 📚 Documentation update - [x] 🎨 Code style/formatting - [x] ♻️ Code refactoring ## Changes Made 1. Update README.md with Railway deprecation note and fixed deployment links 2. Replace fmt.Sprintf with strconv.Itoa for year/month/day formatting in holiday_repository.go 3. Replace fmt.Sprintf with strconv.Itoa in pkg/client/client.go for consistency ## How Has This Been Tested? - [ ] Unit tests - [ ] Integration tests - [x] Manual testing - [ ] Other **Test Configuration:** * Go version: 1.x * Operating System: Linux
6eba886 to
9bfc1a0
Compare
| if filter.Year != nil { | ||
| whereConditions = append(whereConditions, "strftime('%Y', date) = ?") | ||
| args = append(args, fmt.Sprintf("%d", *filter.Year)) | ||
| args = append(args, strconv.Itoa(*filter.Year)) |
There was a problem hiding this comment.
CRITICAL: Lost leading zero padding - SQLite strftime('%m') returns "01"-"12" but strconv.Itoa(1) returns "1", causing month filter to fail silently.
| args = append(args, strconv.Itoa(*filter.Year)) | |
| args = append(args, fmt.Sprintf("%02d", *filter.Month)) |
| if filter.Month != nil { | ||
| whereConditions = append(whereConditions, "strftime('%m', date) = ?") | ||
| args = append(args, fmt.Sprintf("%02d", *filter.Month)) | ||
| args = append(args, strconv.Itoa(*filter.Month)) |
There was a problem hiding this comment.
CRITICAL: Lost leading zero padding - SQLite strftime('%m') returns "01"-"12" but strconv.Itoa(1) returns "1", causing month filter to fail silently.
| args = append(args, strconv.Itoa(*filter.Month)) | |
| args = append(args, fmt.Sprintf("%02d", *filter.Month)) |
| if filter.Day != nil { | ||
| whereConditions = append(whereConditions, "strftime('%d', date) = ?") | ||
| args = append(args, fmt.Sprintf("%02d", *filter.Day)) | ||
| args = append(args, strconv.Itoa(*filter.Day)) |
There was a problem hiding this comment.
CRITICAL: Lost leading zero padding - SQLite strftime('%d') returns "01"-"31" but strconv.Itoa(1) returns "1", causing day filter to fail silently.
| args = append(args, strconv.Itoa(*filter.Day)) | |
| args = append(args, fmt.Sprintf("%02d", *filter.Day)) |
| q := req.URL.Query() | ||
| if year != nil { | ||
| q.Add("year", fmt.Sprintf("%d", *year)) | ||
| q.Add("year", strconv.Itoa(*year)) |
There was a problem hiding this comment.
WARNING: Inconsistent with server expectation - the server's SQLite query expects zero-padded month ("01"-"12") but this sends unpadded ("1"-"12"). This may cause filtering to fail.
| q.Add("year", strconv.Itoa(*year)) | |
| q.Add("year", fmt.Sprintf("%d", *year)) |
| } | ||
| if month != nil { | ||
| q.Add("month", fmt.Sprintf("%d", *month)) | ||
| q.Add("month", strconv.Itoa(*month)) |
There was a problem hiding this comment.
WARNING: Inconsistent with server expectation - the server's SQLite query expects zero-padded month ("01"-"12") but this sends unpadded ("1"-"12"). This may cause filtering to fail.
| q.Add("month", strconv.Itoa(*month)) | |
| q.Add("month", fmt.Sprintf("%02d", *month)) |
Code Review SummaryStatus: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Other Observations (not in diff)No other issues found in unchanged code. Files Reviewed (3 files)
Note: This is a Go project using SQLite. The changes replace |
There was a problem hiding this comment.
Pull request overview
This PR updates deployment documentation links/notes and refactors some integer-to-string formatting from fmt.Sprintf to strconv.Itoa in both the Go SDK client and the holiday repository query builder.
Changes:
- Update README cloud deployment section with a Railway deprecation note and refreshed Railway/Render links.
- Replace
fmt.Sprintf("%d", ...)withstrconv.Itoa(...)for query parameters inpkg/client. - Replace
fmt.Sprintfwithstrconv.Itoainholiday_repositoryfilter argument construction.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/client/client.go | Uses strconv.Itoa for year/month query parameters in the SDK client. |
| internal/repository/holiday_repository.go | Refactors year/month/day filter argument formatting for SQLite strftime-based filtering. |
| README.md | Updates cloud deployment documentation (Railway note/link, Render deploy link). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if filter.Month != nil { | ||
| whereConditions = append(whereConditions, "strftime('%m', date) = ?") | ||
| args = append(args, fmt.Sprintf("%02d", *filter.Month)) | ||
| args = append(args, strconv.Itoa(*filter.Month)) | ||
| } |
There was a problem hiding this comment.
strftime('%m', date) in SQLite returns a zero-padded month string (e.g., "01"). Switching from fmt.Sprintf("%02d", *filter.Month) to strconv.Itoa(*filter.Month) changes the bound parameter to "1", which will no longer match and breaks month filtering (including “this month” and month query param behavior). Consider restoring zero-padding or changing the SQL to compare integers (e.g., CAST(strftime('%m', date) AS INTEGER) = ?) and bind an int.
| if filter.Day != nil { | ||
| whereConditions = append(whereConditions, "strftime('%d', date) = ?") | ||
| args = append(args, fmt.Sprintf("%02d", *filter.Day)) | ||
| args = append(args, strconv.Itoa(*filter.Day)) | ||
| } |
There was a problem hiding this comment.
Same issue as month: strftime('%d', date) returns a zero-padded day-of-month string (e.g., "05"). Using strconv.Itoa(*filter.Day) will bind "5" and the day filter won’t match. Use zero-padding again or compare numerically via CAST(strftime('%d', date) AS INTEGER) = ?.
|
|
||
| #### Railway | ||
| [](https://railway.app/template/your-template-id) | ||
| > **Note:** Railway telah menghentikan dukungan untuk template deployment gratis. Silakan gunakan opsi lain di bawah ini atau deploy secara manual. |
There was a problem hiding this comment.
The README is otherwise written in English in this section, but the newly added Railway deprecation note is in Indonesian. For consistency and readability for the broader audience, consider translating this note to English or providing bilingual text.
| > **Note:** Railway telah menghentikan dukungan untuk template deployment gratis. Silakan gunakan opsi lain di bawah ini atau deploy secara manual. | |
| > **Note:** Railway has discontinued support for free deployment templates. Please use one of the options below or deploy manually. (Catatan: Railway telah menghentikan dukungan untuk template deployment gratis. Silakan gunakan opsi lain di bawah ini atau deploy secara manual.) |
Update deployment docs and refactor string formatting to strconv
Description
Updates README.md with Railway deprecation note and fixes deployment links. Refactors string formatting from fmt.Sprintf to strconv.Itoa for consistency.
Type of Change
Changes Made
How Has This Been Tested?