chore: Bump min go version and use typeassert#35
Conversation
WalkthroughRefactors type assertions to use a generic reflective helper reflect.TypeAssert across decoder, encoder, and tests. Decoder and tests replace direct interface assertions with TypeAssert. Encoder’s isZero retrieves the IsZero() bool result via TypeAssert on a reflect.Value return. No public APIs changed. Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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 (
|
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 focuses on a maintenance update and a code refactoring effort. It upgrades the project's Go version to 1.25, ensuring compatibility with the latest language features and improvements. Concurrently, it modernizes the codebase by replacing traditional interface{}.(Type) type assertions with reflect.TypeAssert where reflect.Value is involved, enhancing type safety and leveraging newer Go reflection capabilities.
Highlights
- Go Version Upgrade: The Go module and continuous integration (CI) configuration have been updated to utilize Go version 1.25, aligning the project with the latest stable release.
- Type Assertion Refactoring: Direct type assertions using
value.Interface().(Type)have been systematically replaced with the more modern and type-safereflect.TypeAssertfunction acrossdecoder.goandencoder_test.go. This change leverages Go's generics to provide compile-time type checking for reflection-based assertions.
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. ↩
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #35 +/- ##
=======================================
Coverage 96.96% 96.97%
=======================================
Files 4 4
Lines 758 760 +2
=======================================
+ Hits 735 737 +2
Misses 12 12
Partials 11 11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request is a great initiative to modernize the codebase by upgrading to Go 1.25 and adopting reflect.TypeAssert. The changes are generally in the right direction. However, there's a recurring issue with how reflect.TypeAssert is used. The boolean ok it returns is consistently ignored. This is problematic because reflect.TypeAssert is equivalent to a two-value type assertion (val, ok := i.(T)), which doesn't panic on failure but returns a zero value and ok=false. The original code used single-value assertions (i.(T)) which panic on failure. By ignoring ok, the new code can lead to nil-pointer dereference panics or, worse, silent bugs where an unexpected zero value is used. My review comments focus on correcting this pattern to ensure the code is robust and behaves as expected.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
encoder_test.go (1)
474-476: Tests: solid migration to reflect.TypeAssert; consider asserting the ok for clearer failures.Using reflect.TypeAssert in tests removes Interface() allocations and mirrors production paths. For even clearer diagnostics when a registration accidentally passes the wrong type, consider checking ok and failing fast with a descriptive message (small nit, since these sites are expected to succeed).
Example pattern you can apply to these sites:
- tv, _ := reflect.TypeAssert[time.Time](value) - return tv.Format(time.RFC3339Nano) + if tv, ok := reflect.TypeAssert[time.Time](value); ok { + return tv.Format(time.RFC3339Nano) + } + t.Fatalf("internal: expected time.Time, got %v", value.Type()) + return ""And similarly for:
- line 525: *CustomTime
- lines 549/580/786: time.Duration
- lines 736/762: time.Time
Note: reflect.TypeAssert is available starting in Go 1.25. (tip.golang.org)
Also applies to: 525-526, 549-552, 580-583, 736-738, 762-764, 786-788
decoder.go (2)
496-498: Use the ok result from TypeAssert to avoid a potential nil-interface method call.In the unlikely event m.IsValid is out of sync with the actual value, um would be a typed-nil interface and calling UnmarshalText would panic. Guarding with ok provides a safer, clearer error.
Apply:
- um, _ := reflect.TypeAssert[encoding.TextUnmarshaler](u) - if err := um.UnmarshalText([]byte(value)); err != nil { + um, ok := reflect.TypeAssert[encoding.TextUnmarshaler](u) + if !ok { + return ConversionError{ + Key: path, Type: t, Index: key, + Err: fmt.Errorf("expected %v to implement encoding.TextUnmarshaler", u.Type()), + } + } + if err := um.UnmarshalText([]byte(value)); err != nil {
579-581: Mirror the TypeAssert ok check on the non-slice path for consistency and safety.Same reasoning as above; this avoids a panic path and returns a structured ConversionError instead.
- um, _ := reflect.TypeAssert[encoding.TextUnmarshaler](u) - if err := um.UnmarshalText([]byte(val)); err != nil { + if um, ok := reflect.TypeAssert[encoding.TextUnmarshaler](u); ok { + if err := um.UnmarshalText([]byte(val)); err != nil { return ConversionError{ Key: path, Type: t, Index: -1, Err: err, } - } + } + } else { + return ConversionError{ + Key: path, Type: t, Index: -1, + Err: fmt.Errorf("expected %v to implement encoding.TextUnmarshaler", u.Type()), + } + }
📜 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 ignored due to path filters (4)
.github/workflows/benchmark.ymlis excluded by!**/*.yml.github/workflows/linter.ymlis excluded by!**/*.yml.github/workflows/test.ymlis excluded by!**/*.ymlgo.modis excluded by!**/*.mod
📒 Files selected for processing (3)
decoder.go(5 hunks)encoder.go(1 hunks)encoder_test.go(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
📚 Learning: 2024-07-01T15:49:51.308Z
Learnt from: luk3skyw4lker
PR: gofiber/storage#1342
File: clickhouse/clickhouse.go:106-107
Timestamp: 2024-07-01T15:49:51.308Z
Learning: The `IsZero` function for `time.Time` structs in the ClickHouse storage driver was returning false even for zero value `time.Time` structs. As a result, a different verification logic was implemented to avoid bugs.
Applied to files:
encoder.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). (2)
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: unit (1.25.x, windows-latest)
🔇 Additional comments (2)
encoder.go (1)
64-65: Please verify Go version enforcementI wasn’t able to locate any
go.modorgo.workfile in the repository, nor any GitHub Actions workflows under.github/workflows, so it’s unclear whether CI or local builds are pinned to Go 1.25 or newer. Before merging, please confirm that:
- A
go.modorgo.workat the project root specifiesgo 1.25(or higher).- All CI pipelines (e.g. GitHub Actions, other runners) are configured to use Go 1.25+.
This ensures the use of
reflect.TypeAssert(introduced in Go 1.25) will work reliably both locally and on CI.decoder.go (1)
625-626: LGTM: centralizing interface checks on reflect.TypeAssert reduces allocations and simplifies isTextUnmarshaler.This refactor is faithful to the original logic while leveraging the new stdlib helper; it should be a net win for hot decode paths that hit TextUnmarshaler frequently. (tip.golang.org)
Also applies to: 632-632, 644-644, 654-654, 660-660
|
@gofiber/maintainers pls review |
Signed-off-by: Juan Calderon-Perez <835733+gaby@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request attempts to upgrade Go to version 1.25 and replace traditional type assertions with reflect.TypeAssert. However, the PR contains critical issues that prevent it from being merged.
Key issues:
- Go 1.25 does not exist as a released version
- The
reflect.TypeAssertfunction does not exist in Go's standard library - The code will not compile with these changes
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Updates Go version to non-existent 1.25 |
| .github/workflows/test.yml | Updates CI to use non-existent Go 1.25 version in test matrix and codecov condition |
| .github/workflows/linter.yml | Updates linter workflow to use non-existent Go 1.25 version |
| .github/workflows/benchmark.yml | Updates benchmark workflow to use non-existent Go 1.25 version |
| encoder.go | Replaces type assertion with non-existent reflect.TypeAssert function in isZero |
| decoder.go | Replaces multiple type assertions with non-existent reflect.TypeAssert function throughout decoder logic |
| encoder_test.go | Replaces type assertions with non-existent reflect.TypeAssert function in test cases and benchmarks |
Critical Problems:
- Go 1.25 does not exist - As of January 2025, the latest stable Go version is 1.24. The version specified throughout this PR is invalid.
reflect.TypeAssertis not a real function - The Go standard library'sreflectpackage does not contain aTypeAssertfunction. This appears to be either a misunderstanding or an assumption about a future API that doesn't exist.- Code will not compile - All changes using
reflect.TypeAssertwill result in compilation errors.
This PR cannot be merged in its current state and requires a complete revision of the approach.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@ReneWerner87 can we merge this? |
Summary
reflect.TypeAssertTesting
go test ./...https://chatgpt.com/codex/tasks/task_e_68a6f89e1c2c8326b880eb6f82f058b8
Summary by CodeRabbit
Refactor
Tests