Skip to content
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.

Sign up for GitHub

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

feat(depinject): support ignored fields in input structs #22409

Closed
wants to merge 4 commits into from
Closed

feat(depinject): support ignored fields in input structs #22409

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e46696d
feat(depinject): support ignore tag in input structs
kocubinski Nov 1, 2024
cd90eea
clean up
kocubinski Nov 1, 2024
58698d5
moar cleanup
kocubinski Nov 1, 2024
64db97b
changelog entry
kocubinski Nov 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
feat(depinject): support ignore tag in input structs
  • Loading branch information
@kocubinski
kocubinski committed Nov 1, 2024
commit e46696df1172fd5e58779dc58abb522eed011dc2
14 changes: 14 additions & 0 deletions depinject/binding_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,3 +300,17 @@ func TestBindingInterfaceTwoModuleScopedAndGlobalBinding(t *testing.T) {
IsResolvedModuleScope(t, pond, moduleC, "Marbled")
IsResolvedInGlobalScope(t, pond, "Marbled")
}

func TestIgnoredField(t *testing.T) {
t.Parallel()
cfg := struct {
depinject.In
TheDuck Duck
IgnoredField bool `ignored:"true"`
DuckAgain Duck
DuckAgainAgain Duck
}{}

err := depinject.Inject(depinject.Provide(ProvideMallard), &cfg)
require.NoError(t, err)
Comment on lines +306 to +315
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance test coverage with additional assertions

While the test verifies successful injection, it should also validate:

  1. The ignored field remains unmodified
  2. The Duck fields are properly injected with Mallard instances

Consider enhancing the test with these assertions:

 func TestIgnoredField(t *testing.T) {
     t.Parallel()
     cfg := struct {
         depinject.In
         TheDuck        Duck
         IgnoredField   bool `ignored:"true"`
         DuckAgain      Duck
         DuckAgainAgain Duck
-    }{}
+    }{
+        IgnoredField: true, // Set initial value
+    }

     err := depinject.Inject(depinject.Provide(ProvideMallard), &cfg)
     require.NoError(t, err)
+    
+    // Verify ignored field remains unchanged
+    require.True(t, cfg.IgnoredField)
+    
+    // Verify all Duck fields are properly injected
+    require.IsType(t, Mallard{}, cfg.TheDuck)
+    require.IsType(t, Mallard{}, cfg.DuckAgain)
+    require.IsType(t, Mallard{}, cfg.DuckAgainAgain)
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
cfg := struct {
depinject.In
TheDuck Duck
IgnoredField bool `ignored:"true"`
DuckAgain Duck
DuckAgainAgain Duck
}{}
err := depinject.Inject(depinject.Provide(ProvideMallard), &cfg)
require.NoError(t, err)
cfg := struct {
depinject.In
TheDuck Duck
IgnoredField bool `ignored:"true"`
DuckAgain Duck
DuckAgainAgain Duck
}{
IgnoredField: true, // Set initial value
}
err := depinject.Inject(depinject.Provide(ProvideMallard), &cfg)
require.NoError(t, err)
// Verify ignored field remains unchanged
require.True(t, cfg.IgnoredField)
// Verify all Duck fields are properly injected
require.IsType(t, Mallard{}, cfg.TheDuck)
require.IsType(t, Mallard{}, cfg.DuckAgain)
require.IsType(t, Mallard{}, cfg.DuckAgainAgain)

}
1 change: 1 addition & 0 deletions depinject/provider_desc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ type providerDescriptor struct {
type providerInput struct {
Type reflect.Type
Optional bool
Ignored bool
}

type providerOutput struct {
Expand Down
13 changes: 13 additions & 0 deletions depinject/struct_args.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,13 @@ func structArgsInTypes(typ reflect.Type) ([]providerInput, error) {
continue
}

var ignored bool
_, found := f.Tag.Lookup("ignored")
if found {
continue
// ignored = true
}

var optional bool
optTag, found := f.Tag.Lookup("optional")
if found {
Expand All @@ -126,6 +133,7 @@ func structArgsInTypes(typ reflect.Type) ([]providerInput, error) {
res = append(res, providerInput{
Type: f.Type,
Optional: optional,
Ignored: ignored,
})
}
return res, nil
Expand Down Expand Up @@ -170,6 +178,11 @@ func buildIn(typ reflect.Type, values []reflect.Value) (reflect.Value, int, erro
if f.Type.AssignableTo(isInType) {
continue
}
_, found := f.Tag.Lookup("ignored")
if found {
continue
}

Comment on lines +180 to +184
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider extracting the ignored tag check into a helper function.

The ignored tag check is duplicated between structArgsInTypes and buildIn. Consider extracting it into a helper function to follow the DRY principle.

+func isIgnoredField(f reflect.StructField) bool {
+    _, found := f.Tag.Lookup("ignored")
+    return found
+}

 func buildIn(typ reflect.Type, values []reflect.Value) (reflect.Value, int, error) {
     // ...
-    _, found := f.Tag.Lookup("ignored")
-    if found {
+    if isIgnoredField(f) {
         continue
     }
     // ...
 }

 func structArgsInTypes(typ reflect.Type) ([]providerInput, error) {
     // ...
-    _, found := f.Tag.Lookup("ignored")
-    if found {
+    if isIgnoredField(f) {
         continue
     }
     // ...
 }

Committable suggestion skipped: line range outside the PR's diff.

if !res.Elem().Field(i).CanSet() {
return reflect.Value{}, 0, fmt.Errorf("depinject.In struct %s on package %s can't have unexported field", res.Elem().String(), f.PkgPath)
}
Expand Down