Refactor textRows.readRow to improve readability - #1123
Conversation
Refactor textRows.readRow to improve readability: * simplified flow: * reduced maximum indent level * return error early * remove 3 'continue' * remove one type cast * remove one obsolete comment about readLengthEncodedString * -6 lines of code
| } | ||
| // If parseDateTime failed, leave as []byte | ||
| } | ||
| return err // err != nil |
There was a problem hiding this comment.
isn't err ignored now?
Seems like this should also have a test case.
There was a problem hiding this comment.
Okay, I just saw the comment above.
This is a semantic change, which I disagree with. We should not introduce any silent fallback behavior. The user requested a parsed time and should receive an error, not something unexpected.
There was a problem hiding this comment.
Well, it looks like I misunderstood the original code. And this shows that it must be refactored to make it easier to follow 😉 .
I will fix my code once the test suite is extended to cover that case.
There was a problem hiding this comment.
This still must be changed before I can approve this change.
methane
left a comment
methane
left a comment
There was a problem hiding this comment.
I prefer one more indent (e.g. put switch statement inside if mc.parseTime {, instead of duplicated dest[i] = b.
But this is matter of taste. The current PR code is OK too.
Co-authored-by: Inada Naoki <songofacandy@gmail.com>
| } | ||
| // If parseDateTime failed, leave as []byte | ||
| } | ||
| return err // err != nil |
There was a problem hiding this comment.
This still must be changed before I can approve this change.
3 participants
Description
Refactor textRows.readRow to improve readability:
Checklist