feat: migrate logger generics#186
Conversation
WalkthroughAdds a new v3 migration (MigrateLoggerGenerics) that rewrites legacy logger APIs to their generic forms and registers it in the migrations list for versions >=2.0.0-0 and <4.0.0-0. Includes unit tests validating replacements and migration output. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant CLI as CLI Command
participant Mig as DoMigration
participant V3 as MigrateLoggerGenerics
participant FS as Filesystem
Dev->>CLI: run migrate (v3 range)
CLI->>Mig: DoMigration(curr, target)
Mig->>Mig: Check version constraints
alt In range
Mig->>V3: Call MigrateLoggerGenerics(cmd, cwd, curr, target)
V3->>FS: internal.ChangeFileContent(cwd, regex replacements)
alt Changes applied
V3-->>CLI: print "Migrating logger generics"
V3-->>Mig: nil
else No changes
V3-->>Mig: nil
end
else Out of range
Mig-->>CLI: skip
end
CLI-->>Dev: exit (success or error)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ReneWerner87, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a new migration script designed to update existing logger interfaces and functions to support Go generics. This ensures compatibility with newer Go features and standardizes logger usage across the codebase, making the logging system more flexible and type-safe.
Highlights
- New Migration Added: A new migration,
MigrateLoggerGenerics, has been integrated into the v3 migration list, ensuring it runs as part of the update process. - Logger Generics Migration Logic: A dedicated migration script has been added to automatically update logger function calls (e.g.,
AllLogger,DefaultLogger,SetLogger,LoggerToWriter) by appending[any]to them, adapting them for generic usage. - Comprehensive Testing: A new test file has been added to thoroughly validate the
MigrateLoggerGenericsfunction, ensuring its correctness and reliability in transforming logger calls.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request adds a migration script to update logger interfaces and functions to use generics, along with corresponding tests. The changes are well-structured and the new migration is correctly registered. I have one suggestion to improve the implementation of the migration script by following Go's best practices for regular expression handling, which will also enhance code readability.
| func MigrateLoggerGenerics(cmd *cobra.Command, cwd string, _, _ *semver.Version) error { | ||
| reAllLogger := regexp.MustCompile(`(\w+)\.AllLogger([^\w\[]|$)`) | ||
| reConfigurableLogger := regexp.MustCompile(`(\w+)\.ConfigurableLogger([^\w\[]|$)`) | ||
| reDefaultLogger := regexp.MustCompile(`(\w+)\.DefaultLogger\(\)`) | ||
| reSetLogger := regexp.MustCompile(`(\w+)\.SetLogger\(`) | ||
| reLoggerToWriter := regexp.MustCompile(`(\w+)\.LoggerToWriter\(`) | ||
|
|
||
| changed, err := internal.ChangeFileContent(cwd, func(content string) string { | ||
| content = reAllLogger.ReplaceAllString(content, `$1.AllLogger[any]$2`) | ||
| content = reConfigurableLogger.ReplaceAllString(content, `$1.ConfigurableLogger[any]$2`) | ||
| content = reDefaultLogger.ReplaceAllString(content, `$1.DefaultLogger[any]()`) | ||
| content = reSetLogger.ReplaceAllString(content, `$1.SetLogger[any](`) | ||
| content = reLoggerToWriter.ReplaceAllString(content, `$1.LoggerToWriter[any](`) | ||
| return content | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to migrate logger generics: %w", err) | ||
| } | ||
| if !changed { | ||
| return nil | ||
| } | ||
|
|
||
| cmd.Println("Migrating logger generics") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
For performance and to follow Go best practices, regular expressions should be compiled only once. You can define them as package-level variables using regexp.MustCompile. This avoids recompiling them every time MigrateLoggerGenerics is called. While the performance impact is likely negligible in this command-line tool context, this change improves code structure and adheres to common Go idioms.
var (
reAllLogger = regexp.MustCompile("(\\w+)\\.AllLogger([^\\w\\[]|$)")
reConfigurableLogger = regexp.MustCompile("(\\w+)\\.ConfigurableLogger([^\\w\\[]|$)")
reDefaultLogger = regexp.MustCompile("(\\w+)\\.DefaultLogger\\(\\)")
reSetLogger = regexp.MustCompile("(\\w+)\\.SetLogger\\(")
reLoggerToWriter = regexp.MustCompile("(\\w+)\\.LoggerToWriter\\(")
)
func MigrateLoggerGenerics(cmd *cobra.Command, cwd string, _, _ *semver.Version) error {
changed, err := internal.ChangeFileContent(cwd, func(content string) string {
content = reAllLogger.ReplaceAllString(content, "$1.AllLogger[any]$2")
content = reConfigurableLogger.ReplaceAllString(content, "$1.ConfigurableLogger[any]$2")
content = reDefaultLogger.ReplaceAllString(content, "$1.DefaultLogger[any]()")
content = reSetLogger.ReplaceAllString(content, "$1.SetLogger[any](")
content = reLoggerToWriter.ReplaceAllString(content, "$1.LoggerToWriter[any](")
return content
})
if err != nil {
return fmt.Errorf("failed to migrate logger generics: %w", err)
}
if !changed {
return nil
}
cmd.Println("Migrating logger generics")
return nil
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
cmd/internal/migrations/v3/logger_generics.go (5)
13-19: Scope regex to Fiber log imports to avoid false positives.The current patterns match any pkg alias (e.g., foo.SetLogger().) That risks unintended rewrites for unrelated packages. Consider detecting the aliases used for github.com/gofiber/fiber/v2/log (and v3/log) from the file’s import list and restrict replacements to those aliases.
Apply this diff to make replacements alias-aware with a safe fallback:
func MigrateLoggerGenerics(cmd *cobra.Command, cwd string, _, _ *semver.Version) error { - reAllLogger := regexp.MustCompile(`(\w+)\.AllLogger([^\w\[]|$)`) - reConfigurableLogger := regexp.MustCompile(`(\w+)\.ConfigurableLogger([^\w\[]|$)`) - reDefaultLogger := regexp.MustCompile(`(\w+)\.DefaultLogger\(\)`) - reSetLogger := regexp.MustCompile(`(\w+)\.SetLogger\(`) - reLoggerToWriter := regexp.MustCompile(`(\w+)\.LoggerToWriter\(`) + // Fallback patterns (any alias) in case we cannot resolve import aliases. + fallbackAllLogger := regexp.MustCompile(`(\w+)\.AllLogger([^\w\[]|$)`) + fallbackConfigurableLogger := regexp.MustCompile(`(\w+)\.ConfigurableLogger([^\w\[]|$)`) + fallbackDefaultLogger := regexp.MustCompile(`(\w+)\.DefaultLogger\s*\(\)`) + fallbackSetLogger := regexp.MustCompile(`(\w+)\.SetLogger\s*\(`) + fallbackLoggerToWriter := regexp.MustCompile(`(\w+)\.LoggerToWriter\s*\(`) changed, err := internal.ChangeFileContent(cwd, func(content string) string { - content = reAllLogger.ReplaceAllString(content, `$1.AllLogger[any]$2`) - content = reConfigurableLogger.ReplaceAllString(content, `$1.ConfigurableLogger[any]$2`) - content = reDefaultLogger.ReplaceAllString(content, `$1.DefaultLogger[any]()`) - content = reSetLogger.ReplaceAllString(content, `$1.SetLogger[any](`) - content = reLoggerToWriter.ReplaceAllString(content, `$1.LoggerToWriter[any](`) + // Detect aliases for Fiber's log package from imports; support v2 and v3 paths. + importRE := regexp.MustCompile(`(?m)^\s*(?:([\w_]+)\s+)?\"github\.com/gofiber/fiber/v(2|3)/log\"`) + matches := importRE.FindAllStringSubmatch(content, -1) + var aliases []string + for _, m := range matches { + alias := m[1] + if alias == "" { + alias = "log" // default alias if none is specified + } + aliases = append(aliases, regexp.QuoteMeta(alias)) + } + + // Compile alias-scoped regexes when we have at least one alias; else use fallbacks. + if len(aliases) > 0 { + pkgs := strings.Join(aliases, "|") + reAllLogger := regexp.MustCompile(`\b(` + pkgs + `)\.AllLogger([^\w\[]|$)`) + reConfigurableLogger := regexp.MustCompile(`\b(` + pkgs + `)\.ConfigurableLogger([^\w\[]|$)`) + reDefaultLogger := regexp.MustCompile(`\b(` + pkgs + `)\.DefaultLogger\s*\(\)`) + reSetLogger := regexp.MustCompile(`\b(` + pkgs + `)\.SetLogger\s*\(`) + reLoggerToWriter := regexp.MustCompile(`\b(` + pkgs + `)\.LoggerToWriter\s*\(`) + + content = reAllLogger.ReplaceAllString(content, `$1.AllLogger[any]$2`) + content = reConfigurableLogger.ReplaceAllString(content, `$1.ConfigurableLogger[any]$2`) + content = reDefaultLogger.ReplaceAllString(content, `$1.DefaultLogger[any]()`) + content = reSetLogger.ReplaceAllString(content, `$1.SetLogger[any](`) + content = reLoggerToWriter.ReplaceAllString(content, `$1.LoggerToWriter[any](`) + } else { + // Fallback: apply broad patterns. + content = fallbackAllLogger.ReplaceAllString(content, `$1.AllLogger[any]$2`) + content = fallbackConfigurableLogger.ReplaceAllString(content, `$1.ConfigurableLogger[any]$2`) + content = fallbackDefaultLogger.ReplaceAllString(content, `$1.DefaultLogger[any]()`) + content = fallbackSetLogger.ReplaceAllString(content, `$1.SetLogger[any](`) + content = fallbackLoggerToWriter.ReplaceAllString(content, `$1.LoggerToWriter[any](`) + } return content })Note: This also tolerates optional whitespace before parentheses.
16-18: Allow optional whitespace before parentheses in call patterns.Go permits spaces between the identifier and the call parens (e.g., DefaultLogger ()) even if it’s uncommon. Making the regex tolerant avoids missing those cases.
Apply the focused tweak (already included above), or minimally:
-reDefaultLogger := regexp.MustCompile(`(\w+)\.DefaultLogger\(\)`) -reSetLogger := regexp.MustCompile(`(\w+)\.SetLogger\(`) -reLoggerToWriter := regexp.MustCompile(`(\w+)\.LoggerToWriter\(`) +reDefaultLogger := regexp.MustCompile(`(\w+)\.DefaultLogger\s*\(\)`) +reSetLogger := regexp.MustCompile(`(\w+)\.SetLogger\s*\(`) +reLoggerToWriter := regexp.MustCompile(`(\w+)\.LoggerToWriter\s*\(`)
21-26: Idempotency guard is good; consider documenting regex intent.The [^\w\[] guard prevents double-applying when generics are already present. Consider a brief comment explaining that, so future maintainers don’t “simplify” it and reintroduce double-rewrite risk.
Example:
- content = reAllLogger.ReplaceAllString(content, `$1.AllLogger[any]$2`) + // Guard ensures we don't match when `[...]` already follows the symbol. + content = reAllLogger.ReplaceAllString(content, `$1.AllLogger[any]$2`)
35-36: Print once per changed file set (optional).Printing a single “Migrating logger generics” message is fine. If you later change this to report per-file, consider prefixing with file paths for easier diffing. No change needed now.
13-37: Optional micro: compile once at package scope.Compiling regexes per run is fine here, but if this migration scales across many files in large repos, moving stable patterns to package scope can shave overhead. Low priority.
cmd/internal/migrations/v3/logger_generics_test.go (3)
21-33: Good coverage of primary replacements; consider adding “already generic” cases.Add a second file (or inline content) that already uses AllLogger[any], ConfigurableLogger[any], DefaultLoggerany, etc., and assert that no changes are made and nothing is printed. This verifies idempotency and the [^\w\[] regex guard.
Proposed additional test:
func Test_MigrateLoggerGenerics(t *testing.T) { @@ } +func Test_MigrateLoggerGenerics_NoChanges_Idempotent(t *testing.T) { + t.Parallel() + dir, err := os.MkdirTemp("", "mloggenericstest-idem") + require.NoError(t, err) + defer func() { require.NoError(t, os.RemoveAll(dir)) }() + + _ = writeTempFile(t, dir, `package main +import fiberlog "github.com/gofiber/fiber/v2/log" +var _ fiberlog.AllLogger[any] = (*struct{})(nil) +var _ fiberlog.ConfigurableLogger[any] = (*struct{})(nil) +func main() { + l := fiberlog.DefaultLogger[any]() + fiberlog.SetLogger[any](l) + _ = fiberlog.LoggerToWriter[any](l, fiberlog.LevelInfo) +}`) + + var buf bytes.Buffer + cmd := newCmd(&buf) + require.NoError(t, v3.MigrateLoggerGenerics(cmd, dir, nil, nil)) + assert.Empty(t, buf.String(), "expected no output when no changes are needed") +}
21-33: Optional: guard against false positives by adding a counter-example.If you adopt alias scoping in the migration, add a control case with a different package exposing SetLogger that must not be rewritten.
Example snippet to include in the same test setup:
_ = writeTempFile(t, dir, `package main import zap "go.uber.org/zap" func main() { _ = zap.NewExample() // ensure another import exists // simulate some other lib: foo.SetLogger(...) should remain unchanged foo.SetLogger(nil) }`)Then assert the file still contains "foo.SetLogger(" after migration.
34-47: Assertion mix is fine; prefer require on invariants.Where a failed assertion would make subsequent checks meaningless (e.g., readFile success, presence of “Migrating logger generics”), using require.* improves signal. Minor.
Possible tweaks:
-assert.Contains(t, content, "DefaultLogger[any]()") +require.Contains(t, content, "DefaultLogger[any]()")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
cmd/internal/migrations/lists.go(1 hunks)cmd/internal/migrations/v3/logger_generics.go(1 hunks)cmd/internal/migrations/v3/logger_generics_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
cmd/internal/migrations/lists.go (1)
cmd/internal/migrations/v3/logger_generics.go (1)
MigrateLoggerGenerics(13-37)
cmd/internal/migrations/v3/logger_generics_test.go (1)
cmd/internal/migrations/v3/logger_generics.go (1)
MigrateLoggerGenerics(13-37)
cmd/internal/migrations/v3/logger_generics.go (1)
cmd/internal/helpers.go (1)
ChangeFileContent(38-79)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build (1.25.x, macos-13)
🔇 Additional comments (2)
cmd/internal/migrations/lists.go (2)
45-47: Wire-up looks correct; order makes sense next to other logger migrations.Placing MigrateLoggerGenerics right after MigrateLoggerTags and before MigrateStaticRoutes aligns it with related logger changes and should minimize surprises from subsequent rewrites.
33-79: Migration ordering validated
A search throughcmd/internal/migrations/v3/generic_helpers.goshows thatMigrateGenericHelpersonly targetsParamsIntandQueryIntpatterns—there are no logger-related regexes—soMigrateLoggerGenericsremains the correct next step inlists.go. No action required.
1 participant
Summary
Testing
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_68aaf8dd7e6c8326b772dab0107af213
Summary by CodeRabbit