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

del invalidates cached_property #2084

Merged
merged 1 commit into from Apr 16, 2021
Merged

del invalidates cached_property #2084

merged 1 commit into from Apr 16, 2021

Conversation

davidism
Copy link
Member

I think using del obj.name makes more sense than invalidate_cached_property(obj, "name"), and doesn't require importing a separate function. I don't expect users to be invalidating/deleting non-cached-properties anyway, I don't think the extra check was giving us much.

Updated the test to test get, set, and delete.

cc @singingwolfboy

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

@davidism davidism added this to the 2.0.0 milestone Apr 16, 2021
@davidism davidism merged commit f50fbf5 into master Apr 16, 2021
@davidism davidism deleted the delete-cached-property branch April 16, 2021 02:24
@ThiefMaster
Copy link
Member

Would it make sense to replace our cached_property at some point with the stdlib one, and just keep a backport of it for <3.8?

https://docs.python.org/3/library/functools.html#functools.cached_property

(no idea if the API is exactly the same though)

@davidism
Copy link
Member Author

I forgot about that, but I feel like there was some catch, maybe it was @jab that pointed it out. Or maybe I'm remembering something else.

@jab
Copy link
Member

jab commented Apr 18, 2021

I think I also suggested what @ThiefMaster suggested at some point.

Looking again now, it seems like CPython's implementation is more robust than our implementation (e.g. handles concurrent access from multiple threads, gives better error messages for edge cases (like when __dict__ is immutable), etc.), and better-documented.

Given that, and given the existence of https://pypi.org/project/backports.cached-property/ (code), I like the idea even better now.

@davidism
Copy link
Member Author

I think I'm going to leave it for now. We can revisit it later.

I know we're using a backport for dataclasses too, but that's only for Python 3.6, so the timeframe is shorter. cached_property was introduced in 3.8. Also, I'm not 100% confident about that backport package, it seems to not be following the backports namespace package requirement that other backport packages in that namespace follow, so it might break things. I just don't have the time to confirm right now, and our implementation works fine.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants