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

Possible improvement for Scanner with sqlint flag #239

Open
ghost opened this issue Jul 1, 2024 · 3 comments
Open

Possible improvement for Scanner with sqlint flag #239

ghost opened this issue Jul 1, 2024 · 3 comments

Comments

@ghost
Copy link

ghost commented Jul 1, 2024

Right now, go-enum generates a massive switch statement when using string enums with the flag --sqlint. This is mostly because the switch checks for nil values and integer types other than int64. gocyclo reports a score of 25, which could be a problem for https://goreportcard.com that wants a minimum gocyclo score of 15.

However, the documentation for sql.Scanner states this:

// The src value will be of one of the following types:
//
//    int64
//    float64
//    bool
//    []byte
//    string
//    time.Time
//    nil - for NULL values
Scan(src any) error

I'm a bit confused as to why go-enum has to check for other types such as int and uint64. Is it necessary?

@abice
Copy link
Owner

abice commented Jul 12, 2024

@iamthenoname321 Great question! I don't remember off the top of my head, but I'll look into seeing if there's a way to reduce that generated code.

I'm also open to a PR if you have the time 😉 😉

@ghost
Copy link
Author

ghost commented Jul 13, 2024

Alright, I'll try to work on it on my spare time. Thanks!

@ghost
Copy link
Author

ghost commented Jul 15, 2024

The NewNullEnum functions call Scan directly, which results in the type switch statement becoming extremely huge. Scan not only has to check for int64, string, and []byte (which is what is said in database/sql's documentation), but it also has to check for other integer types, pointers, and even the enum type itself.

// these functions do the exact same thing
func NewNullEnum(val interface{}) (x NullEnum) {
	x.Scan(val) // yes, we ignore this error, it will just be an invalid value.
	return
}

func NewNullEnumStr(val interface{}) (x NullEnumStr) {
	x.Scan(val) // yes, we ignore this error, it will just be an invalid value.
	return
}

I noticed that this is only one of many issues related to the inconsistency between the four SQL flags. Mainly:

  1. Scan allows both int and string columns, even if writing to them could potentially fail because of different types.
  2. What's the use of --sqlnullstr? If --sql and --sqlint are incompatible, why is --sqlnullint and --sqlnullstr fine? If you invoke them together, it doesn't even generate Scan for NullEnumStr.
  3. The NewNullEnum functions delegate concerns to Scan, which should be used solely for unmarshaling SQL types.
  4. Invoking --sql and --sqlint doesn't cause any errors, but the generated code contains duplicate functions and types.

I think an entire overhaul of the SQL code generation is needed. I'd like to hear your opinion on this.

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

No branches or pull requests

1 participant