fix(time): added nullable Time with better json support #44
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #44 +/- ##
==========================================
+ Coverage 75.88% 76.09% +0.21%
==========================================
Files 42 43 +1
Lines 1758 1782 +24
==========================================
+ Hits 1334 1356 +22
- Misses 306 307 +1
- Partials 118 119 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @cnlangzi - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
suggestion (code_refinement): Consider handling specific JSON unmarshal errors for better error reporting.
Handling specific JSON unmarshal errors could provide more context and help in debugging issues related to date format errors or other JSON related issues.
| if err != nil { | |
| return err | |
| if err != nil { | |
| if _, ok := err.(*json.UnmarshalTypeError); ok { | |
| return fmt.Errorf("time data was not in the expected format: %w", err) | |
| } | |
| return fmt.Errorf("error unmarshaling time data: %w", err) | |
| } |
time.go
Outdated
| if data == nil { | ||
| return nil |
There was a problem hiding this comment.
issue (bug_risk): Returning early when 'data' is nil might mask potential bugs.
It's generally safe to let the JSON unmarshalling process handle nil or empty slices as it will correctly interpret them as zero values for the types. This early return could potentially skip necessary processing or error handling that might be added later.
| func TestTimeInSQL(t *testing.T) { | ||
|
|
||
| now := time.Now() | ||
| d, err := sql.Open("sqlite3", "file::memory:") | ||
| require.NoError(t, err) | ||
|
|
||
| _, err = d.Exec("CREATE TABLE `times` (`id` id NOT NULL,`created_at` datetime, PRIMARY KEY (`id`))") | ||
| require.NoError(t, err) | ||
|
|
||
| result, err := d.Exec("INSERT INTO `times`(`id`) VALUES(?)", 10) | ||
| require.NoError(t, err) | ||
|
|
||
| rows, err := result.RowsAffected() | ||
| require.NoError(t, err) | ||
| require.Equal(t, int64(1), rows) | ||
|
|
||
| result, err = d.Exec("INSERT INTO `times`(`id`, `created_at`) VALUES(?, ?)", 20, now) | ||
| require.NoError(t, err) | ||
|
|
||
| rows, err = result.RowsAffected() | ||
| require.NoError(t, err) | ||
| require.Equal(t, int64(1), rows) | ||
|
|
||
| var t10 Time | ||
| err = d.QueryRow("SELECT `created_at` FROM `times` WHERE id=?", 10).Scan(&t10) | ||
| require.NoError(t, err) | ||
|
|
||
| require.EqualValues(t, false, t10.Valid) | ||
|
|
||
| var t20 Time | ||
| err = d.QueryRow("SELECT `created_at` FROM `times` WHERE id=?", 20).Scan(&t20) | ||
| require.NoError(t, err) | ||
|
|
||
| require.EqualValues(t, true, t20.Valid) | ||
| require.EqualValues(t, now.UTC(), t20.Time().UTC()) | ||
|
|
||
| result, err = d.Exec("INSERT INTO `times`(`id`,`created_at`) VALUES(?, ?)", 11, t10) | ||
| require.NoError(t, err) | ||
|
|
||
| rows, err = result.RowsAffected() | ||
| require.NoError(t, err) | ||
| require.Equal(t, int64(1), rows) | ||
|
|
||
| result, err = d.Exec("INSERT INTO `times`(`id`, `created_at`) VALUES(?, ?)", 21, t20) | ||
| require.NoError(t, err) | ||
|
|
||
| rows, err = result.RowsAffected() | ||
| require.NoError(t, err) | ||
| require.Equal(t, int64(1), rows) | ||
|
|
||
| var t11 Time | ||
| err = d.QueryRow("SELECT `created_at` FROM `times` WHERE id=?", 11).Scan(&t11) | ||
| require.NoError(t, err) | ||
|
|
||
| require.EqualValues(t, false, t11.Valid) | ||
|
|
||
| var t21 Time | ||
| err = d.QueryRow("SELECT `created_at` FROM `times` WHERE id=?", 21).Scan(&t21) | ||
| require.NoError(t, err) | ||
|
|
||
| require.EqualValues(t, true, t21.Valid) | ||
| require.EqualValues(t, now.UTC(), t21.Time().UTC()) | ||
|
|
||
| } |
There was a problem hiding this comment.
suggestion (testing): Consider adding tests for error conditions and edge cases.
The current tests cover the basic functionality well, but there are no tests for error conditions such as database connection failures, SQL syntax errors, or invalid time values. Additionally, edge cases like leap seconds or timezone differences are not tested.
| require.EqualValues(t, true, t21.Valid) | ||
| require.EqualValues(t, now.UTC(), t21.Time().UTC()) | ||
|
|
||
| } |
There was a problem hiding this comment.
suggestion (testing): Add tests for JSON serialization and deserialization of null or invalid times.
The tests for JSON handling are good for valid times, but they do not cover cases where the time is null or invalid. This is important to ensure robustness in handling different kinds of input.
|
Here's the code health analysis summary for commits Analysis Summary
|
Changes
Timefor better json support.