Allowing transport layer to be customized #169
Conversation
There was a problem hiding this comment.
Thanks for keep working on this PR!
I've not yet review the entire PR (especially not yet review the application.py), the following would be enough for our walk-through discussion to discuss.
2 high level topics are:
- Whether we would need a DefaultHttpClient in our code path
- Comparing the oauth2.py in this PR to an existing implementation
msal/oauth2cli/oauth2.py
Outdated
| @@ -35,13 +34,12 @@ def __init__( | |||
| self, | |||
| server_configuration, # type: dict | |||
| client_id, # type: str | |||
| http_client, # type: HttpClient | |||
There was a problem hiding this comment.
Historically, that internal variable in this oauth2.py was named session, serving as a strong hint for the caller to provide a session instance, rather than just an http client instance. I would suggest to follow that prevailing tradition.
There was a problem hiding this comment.
According to the current implementation, it is an http_client which can also be a session, session will hold true for session objects only which is what we historically do. But http_client is what we expect, if the passed http client is a session object it still a http_client as we assume it is supposed to have qualities similar to that http_client interface.
There was a problem hiding this comment.
This entire file looks in good shape. And, as part of the final cross-check, I compare it to my async branch, and notice some line(s) are missing/different. A few of them are actually important.
At this point, rather than pointing them out one-by-one in this PR (which would be very time-consuming) and then you need to hand-pick them, we can probably just download this file into your laptop, and then, either run a "diff" to see the difference, or you can just use it and see the tests presumably still pass.
(Note that I've changed my previous session naming to adopt your http_client, so you can use it as a drop-in replacement.)
msal/oauth2cli/oauth2.py
Outdated
| @@ -35,13 +34,12 @@ def __init__( | |||
| self, | |||
| server_configuration, # type: dict | |||
| client_id, # type: str | |||
| http_client, # type: HttpClient | |||
There was a problem hiding this comment.
This entire file looks in good shape. And, as part of the final cross-check, I compare it to my async branch, and notice some line(s) are missing/different. A few of them are actually important.
At this point, rather than pointing them out one-by-one in this PR (which would be very time-consuming) and then you need to hand-pick them, we can probably just download this file into your laptop, and then, either run a "diff" to see the difference, or you can just use it and see the tests presumably still pass.
(Note that I've changed my previous session naming to adopt your http_client, so you can use it as a drop-in replacement.)
Separating everything related to requests to http module. Lot of changes around that. I can walk through all the changes.