Fix issue 377 #475
Fix issue 377 #475
Conversation
| if argumentType.Kind() != reflect.Interface { | ||
| return false, nil | ||
| } | ||
| if argumentType.NumMethod() == 0 { |
There was a problem hiding this comment.
This check doesn't appear in the case 2: branch, can it be removed?
There was a problem hiding this comment.
I'm guessing this check is to keep handlers like func(event any) {} valid. If that's the case, it's worth adding a comment!
There was a problem hiding this comment.
yes.
If the first argument is an interface, the handler may be func(event any) or func(ctx context.Context).
We should accept both.
| return false, nil | ||
| } | ||
| if !contextType.Implements(argumentType) || !argumentType.Implements(contextType) { | ||
| return false, fmt.Errorf("handler takes an interface, but it is not context.Context: %q", argumentType.Name()) |
There was a problem hiding this comment.
Wondering if there's a case where this isn't an error. The trival case is (edit) I think of any way that this would be possible with how json.Marshal gets used later, the types it passes in shouldn't have any methods.any/interface{}, where the .NumMethod() == 0 check above makes sense. Wondering if the current version of the code panics if the first argument is error, or some other interface with one or more methods
|
LGTM! Let me know if I understood the |
Codecov Report
@@ Coverage Diff @@
## main #475 +/- ##
==========================================
+ Coverage 69.74% 70.06% +0.32%
==========================================
Files 21 21
Lines 1203 1216 +13
==========================================
+ Hits 839 852 +13
Misses 297 297
Partials 67 67
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
[](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/aws/aws-lambda-go](https://github.com/aws/aws-lambda-go) | require | patch | `v1.36.0` -> `v1.36.1` | --- ### Release Notes <details> <summary>aws/aws-lambda-go</summary> ### [`v1.36.1`](https://github.com/aws/aws-lambda-go/releases/tag/v1.36.1) [Compare Source](https://github.com/aws/aws-lambda-go/compare/v1.36.0...v1.36.1) ##### What's Changed - Add additional handler type validations. Fixes issue 377 by [@​shogo82148](https://github.com/shogo82148) in [https://github.com/aws/aws-lambda-go/pull/475](https://github.com/aws/aws-lambda-go/pull/475) **Full Changelog**: aws/aws-lambda-go@v1.36.0...v1.36.1 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/markussiebert/cdk-sops-secrets).
](https://renovatebot.com){kind=link}
Issue #, if available:
fixes #377
Description of changes:
rework of #365, but it just accepts
context.Contextas a first argument.see #365 (review)
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.