Skip to content
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

The client class shouldn't use setattr in some cases #3

Open
Viicos opened this issue Jan 5, 2024 · 2 comments
Open

The client class shouldn't use setattr in some cases #3

Viicos opened this issue Jan 5, 2024 · 2 comments

Comments

@Viicos
Copy link

Viicos commented Jan 5, 2024

This specific bit of code:

for attr in self.__attrs__:
val = request_kwargs.pop(attr, sentinel)
if val is sentinel:
continue
setattr(self, attr, val)

can lead to unexpected behavior, in particular in the case of the headers (and probably others, like cookies) attribute. requests defaults to a case insensitive dict, but providing headers in request_kwargs as a simple dict would override this default dict implementation used.

Instead, we should probably call the update method if these attributes have a dict-like interface.

@sergei-maertens
Copy link
Member

Can't we instead document that if you provide the arguments, you need to match the types from requests? And specifically provide the example from the headers that it should be set to a case insensitive dict?

An alternative would be a lookup table from name -> approprivate type to wrap it with, depending on how complex that conversion is. We could silently set up a CaseInsensitiveDict 🤔

@CharString
Copy link
Contributor

#6 doesn't close this, but handles a different surprise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants