Enhance client usage migration for acquire agent patterns#247
Conversation
ReneWerner87
requested review from
efectn,
gaby and
sixcolors
and removed request for
a team
Summary of ChangesHello @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 significantly extends the client usage migration tool to handle a broader range of patterns, particularly focusing on the Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a migration script to update Fiber client usage patterns, specifically for AcquireAgent flows. The implementation relies heavily on regular expressions and line-by-line parsing, which can be fragile. I've identified a couple of bugs in the implementation related to parsing code blocks and handling imports that could lead to incorrect migrations. Additionally, I've suggested improvements to the new tests to align them with the project's existing testing style for better maintainability. Overall, the changes are a good step towards automating migrations, but the identified issues should be addressed to ensure robustness.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new v3 migration that rewrites various Fiber client usage patterns (including AcquireAgent flows), registers it for versions >=2.0.0-0 and <4.0.0-0, and includes helpers and tests. Changes are additive and do not modify existing public signatures outside registration. (48 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cmd/internal/migrations/v3/client_usage_internal_test.go (1)
8-67: Consider addingt.Parallel()for consistency with other tests.Other tests in this package (e.g.,
Test_MigrateClientUsage,Test_MigrateClientUsage_NoChanges) uset.Parallel(). Adding it here would maintain consistency and allow parallel test execution.Also, consider adding negative assertions to verify old patterns are removed (e.g.,
fiber.AcquireAgent,a.Parse()).func Test_rewriteAcquireAgentBlocks(t *testing.T) { + t.Parallel() + content := `package maincmd/internal/migrations/v3/client_usage.go (1)
126-137: Brace counting doesn't account for braces in strings or comments.The brace depth tracking at lines 128-136 (and similarly at 175-184) uses simple string containment checks. This can miscount if a line contains
{or}within string literals or comments (e.g.,fmt.Println("{")).For a migration tool, this edge case could produce malformed output. Consider using a more robust approach or documenting this limitation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/internal/migrations/lists.go(1 hunks)cmd/internal/migrations/v3/client_usage.go(1 hunks)cmd/internal/migrations/v3/client_usage_internal_test.go(1 hunks)cmd/internal/migrations/v3/client_usage_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
cmd/internal/migrations/lists.go (1)
cmd/internal/migrations/v3/client_usage.go (1)
MigrateClientUsage(37-58)
cmd/internal/migrations/v3/client_usage_test.go (1)
cmd/internal/migrations/v3/client_usage.go (1)
MigrateClientUsage(37-58)
cmd/internal/migrations/v3/client_usage.go (1)
cmd/internal/helpers.go (1)
ChangeFileContent(54-110)
⏰ 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). (3)
- GitHub Check: Build (1.25.x, macos-13)
- GitHub Check: Build (1.25.x, windows-latest)
- GitHub Check: Build (1.25.x, macos-latest)
🔇 Additional comments (6)
cmd/internal/migrations/lists.go (1)
42-42: LGTM!The
MigrateClientUsagefunction is correctly integrated into the migration pipeline with the appropriate version range constraints.cmd/internal/migrations/v3/client_usage_test.go (2)
165-219: Test forAcquireAgentrewrite validates the complex transformation.This test covers the most complex migration path involving
AcquireAgent, request setup, header configuration, and struct parsing. The expected output on line 214 is quite long - consider whether this specific assertion might become fragile if the output format changes slightly.
14-102: Comprehensive test coverage for the migration scenarios.The test validates multiple aspects of the client usage migration including imports, method calls, status/body extraction, and error handling transformations. The assertions are thorough and check both presence of new patterns and absence of old ones. Helper functions (
writeTempFile,readFile,newCmd) are properly defined incommon_test.goand accessible to this test.cmd/internal/migrations/v3/client_usage.go (3)
37-58: Migration message printed after changes are written.The message "Migrating client usage" is printed after the files have already been modified. This is slightly misleading from a UX perspective - typically migration messages are shown before or during the operation. This pattern appears consistent with other migrations in the codebase, so it may be intentional.
260-291: Well-structured pattern-based rewrite loop.The use of a slice of pattern/builder pairs with
ReplaceAllStringFuncis a clean approach that makes it easy to add new patterns. The changed flag is correctly tracked across all replacements.
377-402: Import injection handles both grouped and single import styles.Good coverage of the two common import syntaxes. The single import conversion to a block import (line 398) is a nice touch that keeps imports organized.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a migration script to update client usage patterns from an older API to a new one. The changes are extensive and cover many patterns, including the AcquireAgent flow. The implementation uses regex to find and replace code, which is a practical approach for this kind of task. The accompanying tests are comprehensive and cover a wide range of scenarios.
I've found a few areas where the migration script could be made more robust:
- The script can fail to migrate code if comments are present in specific locations.
- One of the regular expressions for parsing assignments is a bit fragile and could be made more specific.
- The migration for error handling is incomplete as it doesn't update the variable declaration from
[]errortoerror, which will leave the code in a non-compilable state.
I've left detailed comments with suggestions for these points. Overall, this is a great addition that will help users migrate their codebases.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
cmd/internal/migrations/v3/client_usage.go (2)
685-722: Import injection logic is better, but could be simplified givenimports.Process
ensureImport/ensureClientImportnow correctly handle multi‑line blocks and single‑line imports, which fixes the earlier bug where multipleimport()blocks could be generated. However, given thatChangeFileContentalways runsimports.Processon the final source, you don’t really need to micro‑manage grouping, ordering, or even whether there are multiple import declarations.A simpler and more robust approach would be:
- If the target import path isn’t already present, just append a plain
import "github.com/gofiber/fiber/v3/client"immediately after thepackageline (and similarly forencoding/base64when needed).- Let
imports.Processnormalize everything (merge blocks, sort, drop unused imports).That would reduce the number of regexes and edge cases you have to maintain here, while still producing clean import sets thanks to the existing goimports pass.
If you’d like to confirm that
imports.Processhandles these cases as expected across Go versions, you can quickly prototype a few source strings with multipleimportforms and run them throughgoimportslocally or via a small Go snippet callingimports.Process.
724-731: Globalerrs→errrewrite is unsafe and currently produces non‑compiling migrations
rewriteClientErrorHandlingis doing several very broad text‑level rewrites:updated := clientErrIfPattern.ReplaceAllString(content, "if err != nil {") updated = clientErrLenPattern.ReplaceAllString(updated, "err != nil") updated = clientErrComparePattern.ReplaceAllString(updated, "err != nil") updated = clientErrMapPattern.ReplaceAllString(updated, `"err": err`) updated = clientErrVarPattern.ReplaceAllString(updated, "err")This has a few concrete problems:
Type error for flows that declare
errs []error.
For patterns like the AcquireAgent struct example, you start with:var ( retCode int retBody []byte errs []error t map[string]any ) // ... resp, err := client.Post(...)After this function runs,
errsbecomeserrin the declaration, yieldingerr []error, butresp, err := client.Post(...)is assigning the second result ofclient.Post, which is anerror(per the v3 client docs), into that variable. (docs.gofiber.io) This is a compile‑time type mismatch in the migrated code.Overly broad identifier rewrite.
clientErrVarPattern(\berrs\b) replaces everyerrstoken in the file, including:
- Unrelated aggregated error slices (e.g.
errs = append(errs, ...)),- Declarations, not just uses near the migrated client patterns,
- Any helper code using
errsfor non‑HTTP concerns.This was already raised in a previous review and remains a real risk: a migration pass rewriting entire projects should avoid such global identifier renames unless it can prove they are tied to the transformed constructs.
len(errs)rewrites can corrupt expressions.
clientErrLenPatternblindly rewriteslen(errs)toerr != nil, andclientErrComparePatterntries to patch uperr != nil > 0, but:
clientErrComparePattern’s pattern (err\s*!=\s*nil\s*>\s*0) looks incorrect and most likely never fires.- Non‑canonical uses like
if len(errs) != 0,if len(errs) > 1, orlog.Println(len(errs))will become syntactically invalid or semantically wrong (e.g.if err != nil != 0).Suggestion
I’d strongly recommend tightening this function rather than letting it operate on arbitrary code:
- Restrict rewrites to known shapes produced by the earlier migration steps, for example:
- Exact matches of
if len(errs) > 0 {→if err != nil {.- Exact matches of
"errs": errs→"err": err.- Optional: specific
panic(errs)/return ..., errspatterns immediately following rewritten client calls.- Either:
- Handle
errs []errordeclarations explicitly by converting them toerr errorbefore you introduceclient.*(..., error)assignments, or- Avoid touching declarations at all and only rename
errsin expressions where you know the new singleerrexists (i.e., within the migrated blocks).Given this runs over entire user codebases and can currently produce non‑compiling output, I’d treat fixing/scope‑limiting this function as a blocker before shipping the migration.
To gauge impact, you can scan a real project for
errs []errorand othererrsusages not tied to Fiber v2 client patterns, then run this migration and trygo buildto see where the rewritten code fails. A small script like the following can help locate problematic sites before and after the migration:#!/bin/bash # Before migration: find potential non-client errs usages rg -nP '\berrs\b' --type=go -C2 # After running the migration, re-run to see how many got renamed to `err` rg -nP '\berrs\b' --type=go -C2 rg -nP '\berr\b' --type=go -C2 | sed -n '1,40p'This should highlight how much unrelated code the current patterns are touching.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/internal/migrations/v3/client_usage.go(1 hunks)cmd/internal/migrations/v3/client_usage_internal_test.go(1 hunks)cmd/internal/migrations/v3/client_usage_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/internal/migrations/v3/client_usage.go (1)
cmd/internal/helpers.go (1)
ChangeFileContent(54-110)
cmd/internal/migrations/v3/client_usage_test.go (1)
cmd/internal/migrations/v3/client_usage.go (1)
MigrateClientUsage(55-76)
⏰ 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-latest)
🔇 Additional comments (1)
cmd/internal/migrations/v3/client_usage_internal_test.go (1)
11-89: Targeted internal test forrewriteAcquireAgentBlockslooks solidNice focused coverage: the test exercises the full AcquireAgent →
client.Posttransformation (imports, headers, URI, status/body extraction, and JSON decode scaffolding) and normalizes formatting viagofmtSource, which keeps the assertion robust against spacing. No changes needed here.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/internal/migrations/v3/client_usage_test.go (1)
472-488: Remove redundant seconderrcheck in generated client.Get patternIn
Test_MigrateClientUsage_RewritesParseBytesFlow's expected output, the secondif err != nilcheck afterresp.StatusCode()andresp.Body()is redundant. Per Fiber v3 client documentation, a single error check immediately afterclient.Get()is sufficient; the response methods are safe to call afterward without re-checking the sameerrvariable.Update the migration to generate:
resp, err := client.Get("https://httpbin.org/json") if err != nil { panic(err) } status := resp.StatusCode() body := resp.Body()instead of duplicating the error check.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/internal/migrations/v3/client_usage.go(1 hunks)cmd/internal/migrations/v3/client_usage_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/internal/migrations/v3/client_usage.go
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/internal/migrations/v3/client_usage_test.go (1)
cmd/internal/migrations/v3/client_usage.go (1)
MigrateClientUsage(56-77)
⏰ 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). (3)
- GitHub Check: Build (1.25.x, macos-latest)
- GitHub Check: Build (1.25.x, windows-latest)
- GitHub Check: Build (1.25.x, macos-13)
🔇 Additional comments (3)
cmd/internal/migrations/v3/client_usage_test.go (3)
165-188: No-change scenario test looks solid
Test_MigrateClientUsage_NoChangescorrectly asserts that a simple handler using onlyfiber.Ctxis left untouched and that the command output buffer stays empty. This is a good guard that the migration is not over-eager.
295-321: AcquireAgent struct rewrite now matches client API typesThe expected output for
Test_MigrateClientUsage_RewritesAcquireAgentStructdeclareserr error(not[]error) and uses:
resp, err := client.Post(...)retCode = resp.StatusCode()retBody = resp.Body()err = resp.JSON(&t)guarded byif err == nil { ... }This aligns with the
client.Postsignature and avoids the earlier[]errorvserrortype mismatch, while preserving status/body capture and structured JSON decoding.
627-633:gofmtSourcehelper is appropriate and keeps assertions stableWrapping
format.SourceingofmtSourcewithrequire.NoErroris a clean way to normalize both input and expected snippets, avoiding test fragility due to formatting differences.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/internal/migrations/v3/client_usage.go (1)
701-729: Consider simplifying import injection logic.The
ensureImportfunction handles three cases manually (import block, single import, package declaration). While functional, this could be simplified sinceimports.Process(fromgolang.org/x/tools/imports) is called afterward in the migration flow (seecmd/internal/helpers.go:53-109).A simpler approach would be to just insert the import statement and let
goimportshandle the formatting and deduplication.Based on past review comments, this could be simplified for better maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/internal/migrations/v3/client_usage.go(1 hunks)cmd/internal/migrations/v3/client_usage_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/internal/migrations/v3/client_usage.go (1)
cmd/internal/helpers.go (1)
ChangeFileContent(54-110)
cmd/internal/migrations/v3/client_usage_test.go (1)
cmd/internal/migrations/v3/client_usage.go (1)
MigrateClientUsage(58-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). (4)
- GitHub Check: Build (1.25.x, macos-latest)
- GitHub Check: Build (1.25.x, windows-latest)
- GitHub Check: Build (1.25.x, macos-13)
- GitHub Check: Build (1.25.x, ubuntu-latest)
🔇 Additional comments (12)
cmd/internal/migrations/v3/client_usage_test.go (3)
96-108: LGTM! Error handling pattern correctly defers response access.The expected output shows the proper pattern: the error is checked first (
if err != nil), and only after confirmingerr == nilareStatusCode()andBody()called. This prevents nil-pointer panics when the client call fails.Based on past review comments, this pattern has been correctly implemented.
313-339: LGTM! Variable declaration correctly updated to singularerrortype.The expected output properly declares
err error(line 317) instead oferr []error, which matches the v3 client API that returns a singleerrorrather than[]error. The migration correctly updates both the declaration and all error handling logic.Based on past review comments, this type mismatch issue has been resolved.
665-671: LGTM! Helper function provides consistent formatting for test assertions.The
gofmtSourcehelper ensures both expected and actual code are formatted consistently usinggo/format, making test comparisons reliable.cmd/internal/migrations/v3/client_usage.go (9)
29-29: LGTM! Variable declaration pattern correctly handles type conversion.The
clientErrsDeclPatternregex properly matcheserrs []errordeclarations, and line 741 inrewriteClientErrorHandlingreplaces them witherr error, ensuring the migrated code uses the correct singular error type.Based on past review comments, this issue has been resolved.
155-163: LGTM! Brace counting logic correctly handles complex structures.The implementation counts all occurrences of
{and}on each line usingstrings.Count, which properly handles lines with multiple braces (like} else {). This is more robust than checking for single brace presence.Based on past review comments, this fragile logic has been strengthened.
101-101: LGTM! Comment handling improves migration resilience.The logic correctly skips comment lines (
strings.HasPrefix(trimmed, "//")) when searching for theRequest()call, preventing false negatives when comments appear between theAcquireAgentandRequestcalls.Based on past review comments, this robustness issue has been addressed.
218-223: LGTM! Consistent assignment operator usage with proper spacing.Both status and body assignments correctly use the
assignOpvariable (which can be:=or=) with proper spacing, ensuring the generated code matches the original assignment style.Based on past review comments, the inconsistent operator issue has been resolved.
37-37: LGTM! Struct assignment pattern uses specific capture groups.The regex captures status and body variables in separate, explicit groups (
([^,]+?)for each), making the pattern more reliable and less prone to parsing errors from formatting variations.Based on past review comments, this fragile regex has been improved.
304-318: LGTM! Config builder ensures deterministic output.The function sorts header keys before building the config string, ensuring consistent and deterministic migration output across runs.
494-534: LGTM! Generated code safely guards response access behind error checks.The replacement builder correctly generates the pattern where status and body are only extracted inside an
if err == nilblock (line 520), preventing nil-pointer panics when the client call fails.
58-79: LGTM! Migration orchestration follows a logical flow.The function applies transformations in the correct order:
- Rewrite client examples and AcquireAgent blocks (lines 60-61)
- Only proceed if changes were made (line 63)
- Normalize error handling globally (line 67)
- Ensure imports via
internal.ChangeFileContentwhich callsimports.ProcessThe early return optimization (line 64) avoids unnecessary processing for files without client usage.
740-748: The broad\berrs\bpattern at line 746 is applied only to files containing old fiber client patterns (e.g.,status, body, errs := client.Bytes()). The migration runs selectively—only when earlier patterns match and rewrite client code—and no othererrsvariables exist in the codebase that could be affected. While the pattern is technically broad, it operates in a controlled context with no actual risk of corrupting unrelated code.Likely an incorrect or invalid review comment.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive migration script for updating fiber client usage patterns to the new v3 API. The use of regex-based rewriting is a practical approach for this kind of codemod. The changes are extensive and cover many different usage patterns, including the complex AcquireAgent flow. The addition of thorough tests is excellent and crucial for such a fragile task.
I've found a critical issue with how TLSConfig is migrated, which changes per-request settings to global ones. I've also included some suggestions to improve the robustness and maintainability of the migration script. Please see my detailed comments below.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
cmd/internal/migrations/v3/client_usage.go (2)
701-738: SimplifyensureImportby relying onimports.Processinstead of manual rewrites
ensureImportmanually rewrites import blocks and single-line imports using regexes andReplaceAllString, which is relatively complex and can produce multiple import blocks or unusual layouts, especially when there are several single-line imports.Since
ChangeFileContentalready runsimports.Processon the final source, you can take a much simpler and safer approach: just inject the needed import after thepackageline (if not already present) and letgoimportshandle grouping, sorting, and deduplication.For example:
func ensureImport(content, pkg string) string { - importLiteral := fmt.Sprintf("%q", pkg) - if strings.Contains(content, importLiteral) { - return content - } - - blockRegex := regexp.MustCompile(`(?m)^import \(([^)]*)\)`) // import block - if blockRegex.MatchString(content) { - return blockRegex.ReplaceAllStringFunc(content, func(m string) string { - return strings.Replace(m, ")", "\t"+importLiteral+"\n)", 1) - }) - } - - singleImport := regexp.MustCompile(`(?m)^import\s+\"([^\"]+)\"`) - if matches := singleImport.FindStringSubmatch(content); len(matches) == 2 { - existing := matches[1] - if existing == pkg { - return content - } - return singleImport.ReplaceAllString(content, fmt.Sprintf("import (\n\t%q\n\t%q\n)", existing, pkg)) - } - - packageRegex := regexp.MustCompile(`(?m)^package\s+\w+`) - if match := packageRegex.FindString(content); match != "" { - return strings.Replace(content, match, match+"\n\nimport \""+pkg+"\"", 1) - } - - return "import \"" + pkg + "\"\n" + content + importLiteral := fmt.Sprintf("%q", pkg) + if strings.Contains(content, importLiteral) { + return content + } + packageRegex := regexp.MustCompile(`(?m)^package\s+\w+`) + if match := packageRegex.FindString(content); match != "" { + return strings.Replace(content, match, match+"\n\nimport "+importLiteral, 1) + } + // Fallback for malformed files without a package clause. + return "import " + importLiteral + "\n" + content }
imports.Processwill then normalize imports across blocks, so you avoid edge cases while keeping the helper small and easier to reason about.
24-30: Globalerrs→errnormalization is still very broad and can break unrelated code
rewriteClientErrorHandlingrewrites all occurrences oferrs/len(errs)/"errs": errsin a file once any client migration has happened:
clientErrsDeclPatternchanges everyvar errs []errortovar err error.clientErrIfPattern/clientErrLenPattern/clientErrComparePatternaggressively rewritelen(errs)usages.clientErrVarPattern(\berrs\b) then renames every remainingerrsidentifier toerr.This has a couple of risks:
- Any non-Fiber use of
errs []errorin the same file (e.g. validation error aggregation) will silently becomeerr errorwith all references renamed, changing behavior or even failing to compile.- Expressions like
if len(errs) > 1 { ... }will be turned intoif err != nil > 1 { ... }, which does not matchclientErrComparePattern(it only handles> 0) and leaves an invalid boolean expression in place.- Other patterns like
panic(errs)orreturn errsare also rewritten purely by name, regardless of origin.Given this is an automated migration that can run across whole projects, that blast radius is pretty high.
Consider constraining the normalization more tightly, for example:
- Prefer
clientErrIfPatternfor the commonif len(errs) > 0 { ... }case instead of the genericclientErrLenPattern.- Extend
clientErrComparePattern(or add new targeted patterns) to cover the exact shapes you expect from Fiber client usage, and avoid rewriting arbitrarylen(errs)arithmetic.- Limit
clientErrVarPattern/clientErrsDeclPatternto scopes where you know theerrsslice came from the old Fiber client (e.g. heuristics around the rewrittenfiber.*/AcquireAgentblocks), instead of the entire file.At minimum, it’s worth scanning the repo for other
errs []errorusages to ensure you’re not unintentionally mutating unrelated code when this migration runs.- updated = clientErrLenPattern.ReplaceAllString(updated, "err != nil") - updated = clientErrComparePattern.ReplaceAllString(updated, "err != nil") - updated = clientErrVarPattern.ReplaceAllString(updated, "err") + // TODO: consider narrowing these rewrites to only known Fiber-client error + // handling patterns (or to regions around migrated calls) to avoid touching + // unrelated `errs` usage in the same file. + updated = clientErrLenPattern.ReplaceAllString(updated, "err != nil") + updated = clientErrComparePattern.ReplaceAllString(updated, "err != nil") + updated = clientErrVarPattern.ReplaceAllString(updated, "err")#!/bin/bash # Quick sanity check for potential non-Fiber `errs` usage that this migration # might rewrite. echo "== Declarations of errs []error ==" rg -n '\berrs\s*\[\]error\b' -C3 echo echo "== len(errs) comparisons that are not > 0 (may become invalid after rewrite) ==" rg -n 'len\(\s*errs\s*\)\s*(?!>\s*0)' -C3 echo echo "== Files containing both Fiber client usage and errs slice ==" rg -n 'fiber\.(Get|Head|Post|Put|Patch|Delete)\(' -n rg -n 'fiber\.AcquireAgent\(' -nAlso applies to: 740-748
🧹 Nitpick comments (1)
cmd/internal/migrations/v3/client_usage_test.go (1)
453-527: Consider dropping the redundant secondif err != nilin the parse-bytes flowIn
Test_MigrateClientUsage_RewritesParseBytesFlow, the expected migrated code includes:resp, err := client.Get("https://httpbin.org/json") if err != nil { panic(err) } status := resp.StatusCode() body := resp.Body() if err != nil { panic(err) }
erris not modified between the two checks, so the secondif err != nilis redundant and will never fire if the first one didn’t. It makes the generated code a bit noisier for readers.If preserving two separate error sites from the original (
a.Parseanda.Bytes) isn’t important, you could simplify the generator and expected output to a single check:- resp, err := client.Get("https://httpbin.org/json") - if err != nil { - panic(err) - } - status := resp.StatusCode() - body := resp.Body() - if err != nil { - panic(err) - } + resp, err := client.Get("https://httpbin.org/json") + if err != nil { + panic(err) + } + status := resp.StatusCode() + body := resp.Body()This keeps the behavior but produces slightly cleaner migrated code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/internal/migrations/v3/client_usage.go(1 hunks)cmd/internal/migrations/v3/client_usage_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/internal/migrations/v3/client_usage_test.go (1)
cmd/internal/migrations/v3/client_usage.go (1)
MigrateClientUsage(58-79)
cmd/internal/migrations/v3/client_usage.go (1)
cmd/internal/helpers.go (1)
ChangeFileContent(54-110)
⏰ 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). (3)
- GitHub Check: Build (1.25.x, macos-13)
- GitHub Check: Build (1.25.x, macos-latest)
- GitHub Check: Build (1.25.x, windows-latest)
1 participant
Summary
Testing
Codex Task
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.