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.

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

JTASK-863: fix bug in Option[T].Scan #2

Merged
merged 4 commits into from
Nov 22, 2023
Merged

JTASK-863: fix bug in Option[T].Scan #2

merged 4 commits into from
Nov 22, 2023

Conversation

nojima
Copy link
Collaborator

@nojima nojima commented Nov 21, 2023

No description provided.

@nojima nojima changed the title JTASK-863: fix the error message JTASK-863: fix bug in Option[T].Scan Nov 21, 2023
options.go Outdated
Comment on lines 178 to 192
// Convert between string and []byte
dstType := reflect.TypeOf(dst.value)
srcType := reflect.TypeOf(src)
if isByteSlice(srcType) && dstType.Kind() == reflect.String {
src = string(src.([]byte))
*dst = New(src.(T))
return nil
}
if srcType.Kind() == reflect.String && isByteSlice(dstType) {
src = []byte(src.(string))
*dst = New(src.(T))
return nil
}

return fmt.Errorf("Option[%T].Scan: failed to convert value %#v to type %T", dst.value, src, dst.value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that the dst and src type mismatch problem is not limited to strings.
https://zenn.dev/methane/articles/2023-go-mysql-parse-numbers

If we wanted full SQL Driver support with Option type, the flow would be something like this

  1. if src is nil, dst becomes None
  2. if dst type implements a Scanner interface, use it
  3. if dst type is a primitive (or its defined type), use a huge switch statement to achieve the conversion, just as the sql package does. https://cs.opensource.google/go/go/+/master:src/database/sql/convert.go;l=211?q=convertAssign&ss=go%2Fgo

Or, we may be able to use sql.Null[T] after releasing go 1.22.
golang/go#60370

@nojima
Copy link
Collaborator Author

nojima commented Nov 22, 2023

For now, I decided to copy and use the standard Go library implementation.
If Null[T] is introduced into the standard library in the future and Option[T].Scan can be implemented using it, I would like to move to it.

Copy link
Collaborator

@pddg pddg left a comment

Choose a reason for hiding this comment

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

LGTM

@nojima nojima merged commit 80022c1 into main Nov 22, 2023
2 checks passed
@nojima nojima deleted the JTASK-863 branch April 10, 2024 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants