feat(migrator): implement migration for params tag keys to use uri format#273
feat(migrator): implement migration for params tag keys to use uri format#273ReneWerner87 merged 1 commit intomasterfrom
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 introduces a significant migration to update struct tags from Highlights
Changelog
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 new migration to update params struct tags to uri format, aligning with recent changes in the framework. The implementation correctly uses go/ast and go/parser to modify the tags, and comprehensive test cases are provided to ensure its correctness across various scenarios. Additionally, several existing files have been modernized by adopting newer Go standard library functions such as slices.Contains, maps.Copy, strings.CutPrefix, and strings.SplitSeq. These changes enhance code readability, conciseness, and efficiency. Overall, the changes are well-implemented and contribute positively to the codebase's maintainability and adherence to modern Go practices.
There was a problem hiding this comment.
Pull request overview
Adds a new v3 migration step to rewrite struct tag keys from params to uri (URI-format tags), integrating it into the migration pipeline and expanding test coverage. Also includes several small Go “modernize” refactors across existing migrations/dev tooling.
Changes:
- Add
MigrateParamsTagKeysmigration to rewriteparams:"..."struct tags touri:"...". - Register the new migration in the v3 migration list and add dedicated tests (plus a parser-methods regression test).
- Modernize a few areas using newer stdlib helpers (
maps.Copy,slices.Contains,strings.SplitSeq,min/max,strings.CutPrefix) and add a Makefile target for running modernize.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/internal/migrations/v3/params_tag_keys.go | Implements struct-tag rewrite migration (params → uri). |
| cmd/internal/migrations/v3/params_tag_keys_test.go | Adds tests covering rewrite behavior and no-op case. |
| cmd/internal/migrations/v3/parser_methods_test.go | Adds regression test ensuring parser-method migration doesn’t rewrite tags. |
| cmd/internal/migrations/lists.go | Registers the new migration in the v3 migration sequence. |
| cmd/internal/migrations/v3/session_release.go | Minor refactor using SplitSeq and min/max. |
| cmd/internal/migrations/v3/client_usage.go | Minor refactor using maps.Copy and SplitSeq. |
| cmd/internal/migrations/v3/cache_config.go | Minor refactor using strings.CutPrefix. |
| cmd/dev.go | Minor refactor using slices.Contains. |
| Makefile | Adds modernize target. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tagLiteral, err := strconv.Unquote(field.Tag.Value) | ||
| if err != nil || !strings.Contains(tagLiteral, `params:"`) { | ||
| return true | ||
| } | ||
|
|
||
| updatedTag := strings.ReplaceAll(tagLiteral, `params:"`, `uri:"`) | ||
| if updatedTag == tagLiteral { | ||
| return true | ||
| } | ||
|
|
||
| field.Tag.Value = "`" + updatedTag + "`" |
There was a problem hiding this comment.
rewriteParamsTagKeys does a plain strings.ReplaceAll on params:", which will also rewrite tag keys that merely contain "params" (e.g. xparams:"id" would become xuri:"id"). Consider using a stricter match (e.g. regexp with a word boundary or start/space delimiter) so only the exact params key is renamed.
Also, this always re-emits the tag as a raw backquoted literal (...), which changes the literal form if the original tag used a quoted string and can produce invalid Go if the tag content contains a backquote. Preserving the original quoting style (or using strconv.Quote when needed) would avoid that edge case.
| ## modernize: 🛠 Run gopls modernize | ||
| .PHONY: modernize | ||
| modernize: | ||
| GOTOOLCHAIN=$(GOVERSION) go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest -fix -test=false ./... |
There was a problem hiding this comment.
The modernize target sets GOTOOLCHAIN=$(GOVERSION), but GOVERSION isn’t defined anywhere in this Makefile. This will typically expand to an empty value and won’t select the intended toolchain; use $(TOOLCHAIN) here or define GOVERSION consistently.
| GOTOOLCHAIN=$(GOVERSION) go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest -fix -test=false ./... | |
| GOTOOLCHAIN=$(TOOLCHAIN) go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest -fix -test=false ./... |
No description provided.