Conversation
ccoVeille
left a comment
ccoVeille
left a comment
There was a problem hiding this comment.
LGTM 👍
It's a legitimate use case.
One minor remark anyway.
| Jane,2023-06-15T09:30:00Z,false,75.2,100,2.718,prod;live` | ||
| csvData := `name,created_at,active,score,count,rate,tags,rank | ||
| John,2023-01-02T15:04:05Z,true,98.6,42,3.14,test;debug,1 | ||
| Jane,2023-06-15T09:30:00Z,false,75.2,100,2.718,prod;live,42` |
There was a problem hiding this comment.
Could you add a test about sending a negative number via a CSV into a uint field ?
I feel like it could be useful to validate how the library behaves in such use case.
Also, it would help to avoid regression
There was a problem hiding this comment.
Thanks for the comment. I've added a test for invalid values.
| if expectedMessage != actualMessage { | ||
| t.Errorf("Expected a panic for input '%s' with error message '%s', but got '%s'", input, expectedMessage, actualMessage) | ||
| } | ||
| } | ||
|
|
||
| test(t, "score\nnot-float", `error setting field Score: strconv.ParseFloat: parsing "not-float": invalid syntax`) | ||
| test(t, "rank\n-1", `error setting field Rank: strconv.ParseUint: parsing "-1": invalid syntax`) |
There was a problem hiding this comment.
Here the test are too dependent from the implementation. You can see the error message is about strconv failing to parse.
I'm doing this comment here, but I mean it's interesting to change the implementation, and so the tests
There was a problem hiding this comment.
Never mind ... The whole lib is like that...
It's out of the scope of this PR
shared_test.go
Outdated
| Count int `csv:"count"` | ||
| Rate float32 `csv:"rate"` | ||
| Tags string `csv:"tags"` | ||
| Rank uint `csv:"rank"` |
There was a problem hiding this comment.
Please use a uint8 so you could also use 1000 as an invalid value, and simulate an issue an int64 might have with a huge number
There was a problem hiding this comment.
I've added overflow check for both uint and int.
Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>
2 participants
I've noticed that support for uint was missing, so I've added it in this pull request. Let me know if any changes are needed.