feat(fields): read/set all the fields from the given struct #11
Conversation
Reviewer's Guide by SourceryThis pull request implements a new feature for iterating over struct fields with options for setting and getting values, including type conversion and recursive handling. It also refactors existing code to use a new helper function for reflect value processing and adds comprehensive tests for the new functionality. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @bkrukowski - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| } | ||
|
|
||
| func Iterate(strct any, opts ...Option) (err error) { |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the 'Iterate' function into smaller, more focused functions.
While the Iterate function provides flexibility, it can be simplified to improve readability and maintainability. Consider refactoring it into smaller, more focused functions:
- Extract the setter logic into a separate function:
func applySetterAndPrefill(f reflect.StructField, value any, cfg *config, path []reflect.StructField) (any, bool) {
// Call setter
if cfg.setter != nil {
if newVal, ok := cfg.setter(append(path, f), value); ok {
return newVal, true
}
}
// Set pointer to a zero-value struct
if cfg.prefillNilStructs &&
f.Type.Kind() == reflect.Ptr && f.Type.Elem().Kind() == reflect.Struct &&
reflect.ValueOf(value).IsZero() {
return reflect.New(f.Type.Elem()).Interface(), true
}
return value, false
}- Extract the recursive iteration logic:
func recursiveIterate(f reflect.StructField, value any, cfg *config, path []reflect.StructField) (any, bool, error) {
if cfg.recursive && isStructOrNonNilStructPtr(f.Type, value) {
original := value
if err := iterate(&value, cfg, append(path, f)); err != nil {
return nil, false, fmt.Errorf("%s: %w", f.Name, err)
}
if !reflect.DeepEqual(original, value) {
return value, true, nil
}
}
return value, false, nil
}- Simplify the main
iteratefunction:
func iterate(strct any, cfg *config, path []reflect.StructField) error {
var finalErr error
fn := func(f reflect.StructField, value any) (any, bool) {
if finalErr != nil {
return nil, false
}
if cfg.getter != nil {
cfg.getter(append(path, f), value)
}
value, set := applySetterAndPrefill(f, value, cfg, path)
if set {
return value, true
}
value, set, err := recursiveIterate(f, value, cfg, path)
if err != nil {
finalErr = err
return nil, false
}
return value, set
}
err := intReflect.IterateFields(
strct,
fn,
cfg.convertTypes,
cfg.convertToPtr,
)
if err != nil {
return err
}
return finalErr
}These changes maintain the existing functionality while making the code more modular and easier to understand. Each function now has a more focused responsibility, which should improve maintainability and make future modifications easier.
| } | ||
|
|
||
| //nolint:gocognit,goconst,lll | ||
| func TestIterate(t *testing.T) { |
There was a problem hiding this comment.
issue (complexity): Consider restructuring the TestIterate function to improve readability and maintainability.
The TestIterate function is indeed complex and could benefit from restructuring. While it's comprehensive, its current structure makes it difficult to read and maintain. We suggest using Go's subtests feature more effectively to group related scenarios. This approach will improve readability and maintainability without losing any functionality.
Here's an example of how you could restructure a portion of the test:
func TestIterate(t *testing.T) {
t.Parallel()
t.Run("Setter", func(t *testing.T) {
t.Run("Person", func(t *testing.T) {
t.Run("OK", func(t *testing.T) {
runScenario(t, Person{}, Person{Name: "Jane"}, "", fields.Setter(func(_ fields.Path, _ any) (_ any, set bool) {
return "Jane", true
}))
})
t.Run("OK (convert types)", func(t *testing.T) {
runScenario(t, Person{}, Person{Name: "Jane"}, "",
fields.Setter(func(_ fields.Path, _ any) (_ any, set bool) {
return CustomString("Jane"), true
}),
fields.ConvertTypes(true),
)
})
t.Run("Error (convert types)", func(t *testing.T) {
runScenario(t, Person{}, Person{},
"fields.Iterate: IterateFields: *interface {}: IterateFields: fields_test.Person: field 0 \"Name\": value of type fields_test.CustomString is not assignable to type string",
fields.Setter(func(_ fields.Path, value any) (_ any, set bool) {
return CustomString("Jane"), true
}),
)
})
})
// More grouped scenarios...
})
}
func runScenario(t *testing.T, input, expectedOutput interface{}, expectedError string, options ...fields.Option) {
t.Helper()
err := fields.Iterate(&input, options...)
if expectedError != "" {
require.EqualError(t, err, expectedError)
return
}
require.NoError(t, err)
assert.Equal(t, expectedOutput, input)
}This structure:
- Groups related scenarios under descriptive subtest names.
- Uses a helper function
runScenarioto reduce repetition. - Improves readability by clearly separating different test cases.
- Maintains the relationship between tests while reducing nesting.
Apply this pattern throughout the test function to significantly improve its structure and readability.
Pull Request Test Coverage Report for Build 11134063116Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 11135243522Details
💛 - Coveralls |
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @bkrukowski - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
|
|
||
| //nolint:wrapcheck | ||
| func iterate(strct any, cfg *config, path []reflect.StructField) error { |
There was a problem hiding this comment.
suggestion: Add more comments to explain the logic flow
The iterate function and its helper trySetValue handle complex logic. Consider adding more inline comments to explain the flow and decision points, especially in the trySetValue function. This will improve maintainability and make it easier for other developers to understand and modify the code if needed.
| } | ||
|
|
||
| //nolint:gocognit,goconst,lll | ||
| func TestIterate(t *testing.T) { |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the TestIterate function to improve its structure and readability.
The TestIterate function is comprehensive but overly complex. To improve readability and maintainability without sacrificing test coverage, consider the following refactoring:
- Group test cases by functionality and create separate sub-tests for each group.
- Extract the test case definitions into separate variables or functions.
- Create helper functions for common operations.
Here's an example of how you could start refactoring:
func TestIterate(t *testing.T) {
t.Parallel()
t.Run("Setter", func(t *testing.T) {
t.Run("Simple cases", testSimpleSetterCases)
t.Run("Nested structures", testNestedSetterCases)
t.Run("Embedded structures", testEmbeddedSetterCases)
t.Run("Edge cases", testSetterEdgeCases)
})
}
func testSimpleSetterCases(t *testing.T) {
cases := []struct {
name string
options []fields.Option
input any
output any
error string
}{
// Define simple setter test cases here
}
runSetterTestCases(t, cases)
}
func testNestedSetterCases(t *testing.T) {
// Similar structure to testSimpleSetterCases, but for nested structures
}
func testEmbeddedSetterCases(t *testing.T) {
// Similar structure to testSimpleSetterCases, but for embedded structures
}
func testSetterEdgeCases(t *testing.T) {
// Similar structure to testSimpleSetterCases, but for edge cases
}
func runSetterTestCases(t *testing.T, cases []struct {
name string
options []fields.Option
input any
output any
error string
}) {
for _, c := range cases {
c := c
t.Run(c.name, func(t *testing.T) {
t.Parallel()
input := c.input
err := fields.Iterate(&input, c.options...)
if c.error != "" {
require.EqualError(t, err, c.error)
return
}
require.NoError(t, err)
assert.Equal(t, c.output, input)
})
}
}This refactoring maintains test coverage while improving readability and maintainability. It groups related test cases, reduces nesting, and makes it easier to add or modify test cases in the future.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @bkrukowski - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding benchmarks to measure performance, especially for deeply nested structures.
- Consider introducing more specific error types for better error differentiation.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
|
|
||
| //nolint:wrapcheck | ||
| func iterate(strct any, cfg *config, path []reflect.StructField) error { |
There was a problem hiding this comment.
suggestion: Add explanatory comments to iterate function
This function seems to handle multiple complex operations. Consider adding comments to explain the different steps it's performing. This will improve readability and help future maintainers understand its logic.
// iterate traverses the given struct recursively, applying validation rules
// defined in the config. It uses reflection to inspect struct fields and
// their tags, building a path of nested fields as it goes.
func iterate(strct any, cfg *config, path []reflect.StructField) error {
| } | ||
|
|
||
| //nolint:gocognit,goconst,lll | ||
| func TestIterate(t *testing.T) { |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the TestIterate function to use table-driven tests with subtests.
While the TestIterate function provides comprehensive coverage, its nested structure and numerous scenarios can be challenging to maintain and understand. Consider refactoring to use table-driven tests with subtests. This approach will improve readability and maintainability while retaining thorough test coverage. Here's an example of how you could restructure the test:
func TestIterate(t *testing.T) {
t.Parallel()
tests := []struct {
name string
options []fields.Option
input any
output any
error string
}{
{
name: "Person OK",
options: []fields.Option{
fields.Setter(func(_ fields.Path, _ any) (_ any, set bool) {
return "Jane", true
}),
},
input: Person{},
output: Person{
Name: "Jane",
},
error: "",
},
// Add other test cases here...
}
for _, tt := range tests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
input := tt.input
err := fields.Iterate(&input, tt.options...)
if tt.error != "" {
require.EqualError(t, err, tt.error)
return
}
require.NoError(t, err)
assert.Equal(t, tt.output, input)
})
}
}This structure:
- Keeps all test cases in a single slice, making it easy to add or modify cases.
- Uses subtests to maintain separation between test cases.
- Reduces nesting and improves readability.
- Maintains parallel execution of tests.
Consider breaking down very large test cases into separate functions if they require additional setup or assertions. This approach will make your tests more modular and easier to maintain while preserving the comprehensive coverage you've established.
|
Summary by Sourcery
Implement a new feature for iterating over struct fields with options for setting and getting values, including type conversion and recursive handling. Refactor existing code to utilize a new method for reducing value chains, and add extensive tests to ensure functionality. Update build dependencies to include a new testing library.
New Features:
Enhancements:
Build:
Tests: