-
Notifications
You must be signed in to change notification settings - Fork 621
database: Use database.ParseDialect to accept dialect aliases and return canonical dialect (closes #917) #1021
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
base: main
Are you sure you want to change the base?
Conversation
| d = DialectTurso | ||
| case "starrocks": | ||
| d = DialectStarrocks | ||
| case "dsql": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case is new. The lookup map in NewStore has an entry for it, so we should support it here as well.
| // Dialect is the type of database dialect. | ||
| type Dialect string | ||
|
|
||
| var ErrUnknownDialect = errors.New("unknown dialect") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're returning the same error in two places, I thought it better to have a sentinel error. I debated having an error type instead, but the only data I could think of it holding is the unknown dialect, but the caller already has this.
| // aliases, e.g. "postgres" or "pgx". Use this function to ensure that a [Dialect] instance | ||
| // passed to any function that accepts a [Dialect] is a valid [Dialect]. | ||
| func ParseDialect(s string) (d Dialect, err error) { | ||
| // Every non-error return value from this function must have an entry in the [NewStore] lookup map. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To really prevent drift, instead of having a comment here, what do you think of having a registry? I drafted up a PR in globalchubby#1.
I think #917 highlights the issue that both
Dialectandstringcan represent a dialect, and both are accepted in public APIs. For example, compare the signatures ofgoose.SetDialectandgoose.NewProvider. This is a problem becauseDialectlooks like a valid enum, but it can actually be anystring.Long-term, it might be better to only accept strings in the public API, but that's a breaking change.
This PR adds a helper function
database.ParseDialectthat given astringreturns the correspondingDialectvalue, if any. It's used to fix #917, but by shifting the responsibility to Goose users: users using any public API that accepts aDialectcan obtain a validDialectvalue by callingParseDialect.I'm also wondering what you think these changes imply for
OpenDBDriver? I'm also curious about some of thedriverrewrites there which conflict with the logic inParseDialect, e.g.