-
Notifications
You must be signed in to change notification settings - Fork 23
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
Skip interceptor for methods that are not registered in the server #48
Skip interceptor for methods that are not registered in the server #48
Conversation
Co-authored-by: Xander Johnson <[email protected]>
34d8efa
to
a1329a4
Compare
intr_type = AsyncCountingInterceptor if aio else CountingInterceptor | ||
intr = intr_type() | ||
interceptors = [intr] |
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.
Could this be simplified to:
interceptors = [AsyncCountingInterceptor() if aio else CountiningInterceptor()]
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.
It’s following the pattern used elsewhere in the tests. I agree it’s weird looking though and I forget why it’s like that. There’s something to be said for both simplifying it and for keeping it consistent. I’m fine either way so will leave it up to you.
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.
I have gone ahead and left it as is to be consistent with the other tests. My understanding is that while not many, some of the interceptors accept a parameter in their constructor so this pattern allows passing parameters without repetition. Figured may as well leave them as is, and just stick with it throughout.
Thanks for fixing this @anuraaga ! It’s much appreciated! One more small thing, could you bump the version please? It’s needed for a new release. |
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.
Thanks @d5h! sorry for missing the point in the contributor docs
intr_type = AsyncCountingInterceptor if aio else CountingInterceptor | ||
intr = intr_type() | ||
interceptors = [intr] |
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.
I have gone ahead and left it as is to be consistent with the other tests. My understanding is that while not many, some of the interceptors accept a parameter in their constructor so this pattern allows passing parameters without repetition. Figured may as well leave them as is, and just stick with it throughout.
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #48 +/- ##
==========================================
+ Coverage 94.68% 94.70% +0.02%
==========================================
Files 7 7
Lines 376 378 +2
Branches 55 56 +1
==========================================
+ Hits 356 358 +2
Misses 15 15
Partials 5 5 ☔ View full report in Codecov by Sentry. |
No description provided.