Fix migrate output to show full semver and dedupe migration TODO comments#263
Conversation
ReneWerner87
requested review from
Copilot,
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 enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 effectively addresses its goals of normalizing the version output in fiber migrate and preventing duplicate // TODO: migrate comments. The changes are logical and are accompanied by good tests. I've identified a minor bug in the new comment deduplication logic that occurs in an edge case and have suggested a refactoring to fix it and improve the new function's robustness. I also found a small opportunity to clean up some redundant code. Overall, these are solid improvements to the CLI tool.
| if err != nil { | ||
| return fmt.Errorf("invalid version for \"%s\": %w", opts.TargetVersionS, err) | ||
| } | ||
| opts.TargetVersionS = baseVersion.String() |
There was a problem hiding this comment.
This line correctly normalizes a short version string (like "3") into a full semver string ("3.0.0"). However, since you've also updated the fmt.Sprintf call on line 184 to use targetVersion.String(), which already holds the full semver, this assignment to opts.TargetVersionS has become redundant for the purpose of display. It's also overwritten later if a hash is provided.
You can safely remove this line to make the code a bit cleaner.
WalkthroughNormalize duplicate "TODO: migrate" comments during field formatting, apply the normalization when unquoting fails and when formatting fields, adjust displayed target version to use the parsed version's String(), and add tests for the comment behavior and short target-version handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
⏰ 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)
🔇 Additional comments (3)
✏️ Tip: You can disable this entire section by setting 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/internal/migrations/v3/common.go (1)
268-276:⚠️ Potential issue | 🟡 Minor
hasMigrationMarkershould likely betruehere to deduplicate migration comments.The replacement being built at line 269 is
// TODO: migrate <field>: <val>, which contains a migration marker. If the originalcommentalso contains "TODO: migrate", passingfalseprevents the duplicate from being stripped.Compare with line 609 where
hasMigrationMarkeristruewhen the output contains the marker. For consistency and to fulfill the PR objective of avoiding duplicate TODO comments, this should betrue.Proposed fix
- comment = normalizeMigrationComment(comment, false) + comment = normalizeMigrationComment(comment, true)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/internal/migrations/v3/common.gocmd/migrate.go
⏰ 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 (3)
cmd/internal/migrations/v3/common.go (2)
608-614: LGTM!The logic correctly detects when the field being formatted is already a migration TODO and strips duplicate migration comments from the trailing comment.
616-624: LGTM!The helper function is straightforward and correctly implements the deduplication logic.
cmd/migrate.go (1)
182-184: LGTM!Using
targetVersion.String()ensures the output always displays the full semver (e.g.,3.0.0instead of3), which aligns with the PR objective.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
ReneWerner87
added
☢️ Bug
github-actions
Bot
added
the
📒 Documentation
ReneWerner87
removed
the
📒 Documentation
Motivation
fiber migrateoutput always shows the full semantic version (e.g.3.0.0) even when the user supplies a short target like-t=3.// TODO: migrate ...comments when migration logic already emits the same migration TODO as a field name.Description
baseVersion.String()and print the finaltargetVersion.String()in the migration message incmd/migrate.go.normalizeMigrationCommentincmd/internal/migrations/v3/common.goand call it fromFormatFieldWithCommentand the unquote error path to suppress duplicate// TODO: migratecomments when the field itself already contains such a marker.Test_Migrate_TargetVersionShortincmd/migrate_test.goto cover short-t=3usage, andTestFormatFieldWithComment_SkipsDuplicateMigrationCommentincmd/internal/migrations/v3/common_test.goto validate comment deduplication.cmd/migrate.go,cmd/internal/migrations/v3/common.go,cmd/migrate_test.go,cmd/internal/migrations/v3/common_test.go.Testing
make lint, which completed with0 issues.make test, which executed the full test suite (307 tests) and completed successfully; new tests passed as part of the run.Codex Task
Summary by CodeRabbit
Bug Fixes
Tests