hooks: Recursively search embedded fields for methods #494
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Follow up to alecthomas#493 and 840220c Kong currently supports hooks on embedded fields of a parsed node, but only at the first level of embedding: ``` type mainCmd struct { FooOptions } type FooOptions struct { BarOptions } func (f *FooOptions) BeforeApply() error { // this will be called } type BarOptions struct { } func (b *BarOptions) BeforeApply() error { // this will not be called } ``` This change adds support for hooks to be defined on embedded fields of embedded fields so that the above example would work as expected. Per alecthomas#493, the definition of "embedded" field is adjusted to mean: - Any anonymous (Go-embedded) field that is exported - Any non-anonymous field that is tagged with `embed:""` *Testing*: Includes a test case for embedding an anonymous field in an `embed:""` and an `embed:""` field in an anonymous field.
alecthomas
reviewed
Jan 29, 2025
The 'receivers' parameter helps avoid constant memory allocation as the backing storage for the slice is reused across recursive calls.
abhinav
commented
Jan 29, 2025
Comment on lines
+79
to
+80
| var traverse func(value reflect.Value, receivers []reflect.Value) []reflect.Value | ||
| traverse = func(value reflect.Value, receivers []reflect.Value) []reflect.Value { |
There was a problem hiding this comment.
One thing worth noting:
Adding a receivers parameter here allows the recursive calls to reuse the slice's backing storage
so that when one append gives the slice 2x capacity, that is re-used in following appends
before needing another resize further down the stack.
We could change the nil below to something like make([]reflect.Value, 0, 16), and this iteration/collection would be alloc-free for a recursive depth of 16 (which should be more than enough for 90% of the use cases).
alecthomas
approved these changes
Jan 30, 2025
|
Nice, thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow up to #493 and 840220c
Kong currently supports hooks on embedded fields of a parsed node,
but only at the first level of embedding:
This change adds support for hooks to be defined
on embedded fields of embedded fields so that the above
example would work as expected.
Per #493, the definition of "embedded" field is adjusted to mean:
embed:""Testing:
Includes a test case for embedding an anonymous field in an
embed:""and an
embed:""field in an anonymous field.