-
Notifications
You must be signed in to change notification settings - Fork 45
Feat/middleware pipeline #3
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
Conversation
MIchaelMainer
left a comment
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 need more context on what the plans are for this client.
ddyett
left a comment
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.
Never to early to start adding documentation such as the licensing agreements
| return super().send(request, **kwargs) | ||
|
|
||
| def _middleware_present(self): | ||
| return self._middleware |
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.
do we need this? It seems longer than just saying if self._middleware but I know python standards also have a thing about being explicit. not sure this adds much though.
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 did this for readability. So that the if conditions reads if middleware_present ...
| def test_middleware_pipeline(self): | ||
| url = 'https://proxy.apisandbox.msdn.microsoft.com/svc?url=https://graph.microsoft.com/v1.0/me' | ||
| middlewares = [ | ||
| _AuthMiddleware() |
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.
_AuthMiddleware() [](start = 12, length = 17)
would be good if this test had more than one middleware so you're testing the chaining of the middleware.
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 tested that here
Overview
Adds a middleware pipeline.
Enforces TLS 1.2
Notes
The middleware pipeline is a
linkedlistwith middlewares as nodes. The middlewares inherit fromHTTPAdapter.Creating a middleware
Creating requests with middlewares
closes #2