Repository navigation
use properties to allow more easily changing cookies/headers after init - #274
Conversation
|
Thanks for opening this! This is definitely a question of Requests compatibility which is something we want to maintain. I have no issue with this change but I've never used cookies/cookiejars with Requests so I don't know much in this area. cc @tomchristie |
|
I'm not sure about this change. It seems fairly reasonable to me to force the usage of: client.cookies = httpx.Cookies(LWPCookieJar())Since the type that the property ends up being is an client.cookies = LWPCookieJar()
isinstance(client.cookies, LWPCookieJar). # FalseOn the other hand, perhaps it's worth it for simplicity. I think a good way of reviewing if we want this or not would be to ask the same question of other assignments. Eg. Do we want to support this... client.headers = {...}I guess the answer to that is "yeah, sure", in which case we probably want the same from |
|
It seems like a good idea to me, it's what we already do for a lot of |
|
Okay, then I'm a "yes" to this pull request, but I'd like it to be updated to handle both |
|
I'd be okay with this (coercing property assignments of To address the "second half of the problem" mentioned by @toppk (which does exist if we go down this road, I think?), we should probably update the Client API to document that |
|
@toppk Are you comfortable making the changes requested above yourself or would you like that to be done in a separate PR? |
|
i'll give it a shot. I think we can use the same PR. |
|
added typing information, api documentation, and repeated for headers rebased for a cleaner commit, |
toppk
changed the title
lovelydinosaur
left a comment
lovelydinosaur
left a comment
There was a problem hiding this comment.
Looking great. One minor set of changes and I think we鈥檙e there. Nice one!
| ) -> typing.Optional[CookieTypes]: | ||
| if cookies or self.cookies: | ||
| merged_cookies = Cookies(self.cookies) | ||
| if cookies or self._cookies: |
There was a problem hiding this comment.
On reflection I think we should just use the public self.cookies/self.headers accessors for these cases.
There was a problem hiding this comment.
done. Do you want me to do that in the constructor as well?
There was a problem hiding this comment.
Probably best to make sure the implementations for turning user-input into our wrapped classes are identical for constructor or setter.
There was a problem hiding this comment.
my mypy foo is lacking. I get an error message if I update the construct to use the property.
httpx/client.py:105: error: Incompatible types in assignment (expression has type "Union[Headers, Dict[Any, Any], List[Tuple[Any, Any]], None]", variable has type "Headers")
i've left it as is for now.
|
Also: Can we get two simple unit tests to see that objects are transformed properly :) |
- use properties for Client.headers and Client.cookies - update the Client API docs to document the headers and cookies - add Client model tests for setting headers and cookies from dict - add client cookie test that sets cookies
sethmlarson
left a comment
sethmlarson
left a comment
There was a problem hiding this comment.
Two small review comments then this PR looks good to me.
sethmlarson
left a comment
sethmlarson
left a comment
There was a problem hiding this comment.
As long as CI passes I'll merge! Thanks! :)
|
Hope you'll stick around and try tackling some other issues @toppk! :) |
|
Nice work @toppk and @sethmlarson on this one 馃憤 |
coming from requests, I was used to doing session.cookies = LWPCookieJar()
with httpx, I have to do:
client.cookies = httpx.models.Cookies(LWPCookieJar())
or I can just do this as part of the Client constructor.
This doesn't solve the second half of the problem, where now I have to do:
client.cookies.jar.save()
instead of
session.cookies.save()
But just thought I'd through this patch in there. I suspect there will be others looking to do something similar. This is easy enough to workaround.