String decoding in remote connection #3663
Conversation
Have also confirmed that this test would fail on original implementation of add_header that was fixed in 55ed929
Test method seperately, and then test that RemoteConnection calls it.
|
@davehunt, agree this was cumbersome to test. If my code split out is really undesirable, should it be straight forward to just do the very small fix to the code and rework the tests now I figured out all the mocking. But, it seems to me that _request needs breaking up into smaller methods to allow for easier unit testing, so I figured I might as well start down that path. Putting the header addition on the Request class seemed reasonable, but also see the first commit where I just made it a method on RemoteConnection - in case that's preferable. |
There was a problem hiding this comment.
This looks great @birdsarah! Let me know if you have time to address my comments. If not, I'm happy to merge this as it is.
| if parsed_url.username: | ||
| base64string = base64.b64encode('{0.username}:{0.password}'.format(parsed_url).encode()) | ||
| request.add_header('Authorization', 'Basic {}'.format(base64string).decode()) | ||
| request.add_remote_connection_headers(parsed_url) |
There was a problem hiding this comment.
I like the idea of splitting this out. We could take it a step further though and remove the duplication from lines 450-458. Perhaps a get_remote_connection_headers that takes the URL and a boolean for keep_alive? It's also fine if you want to push this to a later date.
| parsed_url.query, | ||
| parsed_url.fragment) | ||
| ) | ||
| request = Request(cleaned_url) |
There was a problem hiding this comment.
What's the reason for the cleaned_url. Can we not just use the original url here?
|
Sorry for delay - will look at your suggestions in the morning... |
|
Ready! I factored out the other headers declaration so there's no duplication. Please note the following:
|
|
Thanks @birdsarah! |
Fixes #3660
Xin the preceding checkbox, I verify that I have signed the Contributor License Agreement