add Charset() option #1679
add Charset() option #1679
Conversation
WalkthroughThis pull request revises the Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Code
participant Config as Config Struct
Client->>+Config: Charset(charset, collation)
Note right of Config: Update private `charsets` field\nand `Collation` field based on inputs
Config-->>-Client: Updated configuration
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dsn.go (1)
83-83: Consider simplifying the charsets field type.The
charsetsfield is defined as[]stringbut based on theCharset()function implementation and typical MySQL usage, only a single charset is needed for theSET NAMEScommand. Consider simplifying this to juststring.- charsets []string // Connection charset. When set, this will be set in SET NAMES <charset> query + charset string // Connection charset. When set, this will be set in SET NAMES <charset> query
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dsn.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: test (windows-latest, 1.23, mariadb-10.5)
- GitHub Check: test (windows-latest, 1.23, mariadb-10.6)
- GitHub Check: test (windows-latest, 1.23, mariadb-10.11)
- GitHub Check: test (windows-latest, 1.23, mariadb-11.1)
- GitHub Check: test (windows-latest, 1.23, mariadb-11.2)
- GitHub Check: test (windows-latest, 1.23, mariadb-11.4)
- GitHub Check: test (windows-latest, 1.23, 5.7)
- GitHub Check: test (windows-latest, 1.23, 8.0)
- GitHub Check: test (windows-latest, 1.23, 8.4)
- GitHub Check: test (windows-latest, 1.23, 9.0)
- GitHub Check: test (macos-latest, 1.23, mariadb-10.5)
- GitHub Check: test (macos-latest, 1.23, mariadb-10.6)
- GitHub Check: test (macos-latest, 1.23, mariadb-10.11)
- GitHub Check: test (macos-latest, 1.23, mariadb-11.1)
- GitHub Check: test (macos-latest, 1.23, mariadb-11.4)
🔇 Additional comments (1)
dsn.go (1)
138-151: Implementation looks good!The
Charset()function is well-documented and follows the established functional options pattern. The implementation correctly handles both charset-only and charset+collation cases, which aligns with MySQL'sSET NAMEScommand behavior.
| // Charset sets the connection charset and collation. | ||
| // | ||
| // charset is the connection charset. | ||
| // collation is the connection collation. It can be null or empty string. | ||
| // | ||
| // When collation is not specified, `SET NAMES <charset>` command is sent when the connection is established. | ||
| // When collation is specified, `SET NAMES <charset> COLLATE <collation>` command is sent when the connection is established. | ||
| func Charset(charset, collation string) Option { | ||
| return func(cfg *Config) error { | ||
| cfg.charsets = []string{charset} | ||
| cfg.Collation = collation | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for charset parameter.
The function accepts any string as charset without validation. Consider adding validation to ensure the charset is supported by MySQL to prevent runtime errors.
func Charset(charset, collation string) Option {
return func(cfg *Config) error {
+ if charset == "" {
+ return errors.New("charset cannot be empty")
+ }
cfg.charsets = []string{charset}
cfg.Collation = collation
return nil
}
}Additionally, you might want to consider maintaining a list of valid MySQL charsets for validation:
var validCharsets = map[string]bool{
"utf8mb4": true,
"utf8": true,
"latin1": true,
// Add other supported charsets
}
func Charset(charset, collation string) Option {
return func(cfg *Config) error {
if charset == "" {
return errors.New("charset cannot be empty")
}
if !validCharsets[charset] {
return fmt.Errorf("unsupported charset: %s", charset)
}
cfg.charsets = []string{charset}
cfg.Collation = collation
return nil
}
}
Description
Fix #1664.
Checklist
Summary by CodeRabbit
New Features
Refactor