gtalarico / pyairtable

Python Api Client for Airtable
https://pyairtable.readthedocs.io
MIT License
765 stars 138 forks source link

⚡️ Add hashing to core types #322

Closed hynky1999 closed 9 months ago

hynky1999 commented 9 months ago

Both Base and Table primitives have a unique identifiers but neither of them implements __hash__. This is problematic, because user application might want to want to use the primitives in cached function(lru_cache), but they will not work out of the box without __hash__ function.

I wanted to also add __hash_ to RecordDict, however since it's a dict I can't. Is there any reason why RecordDict is not dataclass ?

mesozoic commented 9 months ago

You should be able to use lru_cache with these classes; by default, it will use the object's identity as a hash. Given that Api, Base, etc. are mutable and contain complex state, I'm not sure it's appropriate for two separate instances to be treated as if they are the same object. That could create its own unintended consequences.

If you've run into any specific issues besides passing these into lru_cache, would you mind sharing a minimally reproducible sample of the problem?

Re: RecordDict, previous versions of the library returned dict (prior to implementing types), so we've preserved that for backwards compatibility. We could look into doing that as a breaking change in the future, but we'd probably want to add __getitem__ to reduce the impact on existing library users.

hynky1999 commented 9 months ago

You should be able to use lru_cache with these classes; by default, it will use the object's identity as a hash. Given that Api, Base, etc. are mutable and contain complex state, I'm not sure it's appropriate for two separate instances to be treated as if they are the same object. That could create its own unintended consequences`

Good point, I forgot how stateful the classes are...

If you've run into any specific issues besides passing these into lru_cache, would you mind sharing a minimally reproducible sample of the problem?

Not really, the biggest problem I faced was during retrieving of linked records. Because many linked records are the same in some tables, I just wanted to have some sort of caching (I need to retrieve both schema and records).

Re: RecordDict, previous versions of the library returned dict (prior to implementing types), so we've preserved that for backwards compatibility. We could look into doing that as a breaking change in the future, but we'd probably want to add getitem to reduce the impact on existing library users.

Make sense

Closing the PR as I no longer see a common benefit for most of the users.