psf / requests

A simple, yet elegant, HTTP library.
https://requests.readthedocs.io/en/latest/
Apache License 2.0
52.17k stars 9.33k forks source link

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

Open michal-klimek opened 2 years ago

michal-klimek commented 2 years ago

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 https://github.com/psf/requests/issues/3848#issuecomment-277197592

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 commented 2 years ago

@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 commented 2 years ago
  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