Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor textRows.readRow to improve readability #1123

Closed
wants to merge 4 commits into from
Closed

Refactor textRows.readRow to improve readability #1123

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d4d83cc
Simplify textRows.readRow
dolmen Jun 2, 2020
317d804
textRows.readRow: refactor for more early return style
dolmen Jun 2, 2020
a4c7847
textRows.readRow: simplify switch block
dolmen Jun 2, 2020
5ea471c
packets.go: add empty line for readability
dolmen Jun 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
49 changes: 23 additions & 26 deletions packets.go
View file
Open in desktop
Expand Up @@ -761,40 +761,37 @@ func (rows *textRows) readRow(dest []driver.Value) error {
}

// RowSet Packet
var n int
var isNull bool
pos := 0

for i := range dest {
// Read bytes and convert to string
dest[i], isNull, n, err = readLengthEncodedString(data[pos:])
b, isNull, n, err := readLengthEncodedString(data[pos:])
if err != nil {
return err
}
pos += n
if err == nil {
if !isNull {
if !mc.parseTime {
continue
} else {
switch rows.rs.columns[i].fieldType {
case fieldTypeTimestamp, fieldTypeDateTime,
fieldTypeDate, fieldTypeNewDate:
dest[i], err = parseDateTime(
dest[i].([]byte),
mc.cfg.Loc,
)
if err == nil {
continue
}
default:
continue
}
}

} else {
dest[i] = nil
if isNull {
dest[i] = nil
methane marked this conversation as resolved.
Show resolved Hide resolved
continue
}
if !mc.parseTime {
dest[i] = b // type: []byte
continue
}
switch rows.rs.columns[i].fieldType {

dolmen marked this conversation as resolved.
Show resolved Hide resolved
case fieldTypeTimestamp, fieldTypeDateTime,
fieldTypeDate, fieldTypeNewDate:

t, err := parseDateTime(b, mc.cfg.Loc)
if err == nil {
dest[i] = t // type: time.Time
continue
}
// If parseDateTime failed, leave as []byte
}
return err // err != nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't err ignored now?
Seems like this should also have a test case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still must be changed before I can approve this change.


dest[i] = b // type: []byte
}

return nil
Expand Down