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

LookupDict does not implement full contract of the dict class #6238

Open
michal-klimek opened this issue Sep 15, 2022 · 2 comments
Open

LookupDict does not implement full contract of the dict class #6238

michal-klimek opened this issue Sep 15, 2022 · 2 comments

Comments

@michal-klimek
Copy link

michal-klimek commented Sep 15, 2022

LookupDict absolutely includes all of the dict methods. In particular, it allows the use of __setitem__ and other things from the dict class. It specifically does only one thing, which is override __getitem__ to allow returning None.

So it's not ok to replace the dict with object in this case, even though the requests codebase doesn't use that functionality.

Thanks for reporting this issue though, and I hope you keep reading through the codebase!

Originally posted by @Lukasa in #3848 (comment)

This is unfortunately a false statement. Take a look on the following checks:

In [1]: from requests.status_codes import codes

In [2]: codes
Out[2]: <lookup 'status_codes'>

In [3]: codes.keys()
Out[3]: dict_keys([])

In [4]: list(codes.keys())
Out[4]: []

If LookupDict pretends to quack like a duck it should do so. Or it should not derive from the dict if it works even worse:

In [8]: from requests.status_codes import codes

In [9]: codes["my_code"] = 700

In [10]: list(codes.keys())
Out[10]: ['my_code']

In [11]: codes["my_code"]

In [12]: 
@Ali-Xoerex
Copy link

Ali-Xoerex commented Oct 2, 2022

@Lukasa , @sigmavirus24
LookupDict object inherits dict object , so it's basically a dictionary. However, when setting a key to a value like codes["my_code"] = 700 from @michal-klimek example, that key is inaccessible because __getitem__ and get methods have been overridden only to access the object's attributes.
To resolve this issue, we can override the __setitem__ to write key-value pairs to object's attributes or __dict__.
Do you agree with this solution?

@sigmavirus24
Copy link
Contributor

  1. It doesn't really need to be a dictionary
  2. I don't believe we intended the dictionary to be extendable
  3. If we did want it to be extendable, we'd have made a good API to allow for that

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