-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
tpl/collections: Simplify the reflect usage #14085
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
bep
commented
Oct 23, 2025
- Use helper funcs in hreflect package when possible.
- Use hreflect.ConvertIfPossible to handle conversions when possible.
75842f6 to
c0ed3a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the tpl/collections package to simplify reflection usage by consolidating helper functions into the hreflect package. The main goals are to reduce code duplication and provide a centralized, well-tested implementation of type conversion utilities.
Key changes include:
- Removing local reflection helper functions (
toFloat,toInt,toUint,toString,isNumber, etc.) fromtpl/collections - Implementing a comprehensive
ConvertIfPossiblefunction inhreflectthat handles lossless numeric conversions with proper overflow and precision checks - Adding new conversion helper functions (
ToInt64E,ToFloat64E,ToStringE) tohreflect
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tpl/collections/where.go |
Replaces local helper functions with hreflect equivalents for type checking and conversion |
tpl/collections/where_test.go |
Removes TestToFloat test now covered by hreflect tests |
tpl/collections/reflect_helpers.go |
Removes duplicate helper functions (numberToFloat, convertNumber, isNumber, etc.) in favor of hreflect package |
tpl/collections/collections.go |
Updates to use hreflect conversion functions and fixes type vs kind confusion in Union |
tpl/collections/symdiff_test.go |
Removes extraneous blank line |
common/hreflect/helpers.go |
Implements comprehensive ConvertIfPossible with overflow/precision checking for int, uint, and float conversions |
common/hreflect/convert.go |
Adds new conversion helper functions with both error-returning and panicking variants |
common/hreflect/helpers_test.go |
Expands test coverage for ConvertIfPossible with additional conversion scenarios |
common/hreflect/convert_test.go |
Adds basic tests and benchmarks for new conversion functions |
htesting/hqt/checkers.go |
Adds IsSameNumber checker supporting all numeric types for testing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
c0ed3a3 to
8f73140
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
f4783af to
ecf5011
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dadf23e to
ba188d3
Compare
* Use helper funcs in hreflect package when possible. * Use hreflect.ConvertIfPossible to handle conversions when possible. * Move scratch.go from common/maps to common/hstore to clear cyclic import in the next step. * Move Indirect to hreflect and reimplementing it and adusting the behavior to preserve struct pointers. * Adjust evaluateSubElem used by where and others making the struct pointer method case slightly faster.
ba188d3 to
f7317df
Compare