Refactor textRows.readRow to improve readability#1123
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Uh oh!
There was an error while loading. Please reload this page.
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 | ||
| } | ||
| returnerr// 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.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Inada Naoki <[email protected]>
| } | ||
| // If parseDateTime failed, leave as []byte | ||
| } | ||
| returnerr// err != nil |
There was a problem hiding this comment.
This still must be changed before I can approve this change.
dolmen commented Jan 18, 2023
3 participants
Description
Refactor textRows.readRow to improve readability:
Checklist