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

Cache hash codes #426

Merged
merged 15 commits into from Aug 20, 2018
Merged

Cache hash codes #426

merged 15 commits into from Aug 20, 2018

Conversation

@gabbard
Copy link
Member

@gabbard gabbard commented Aug 19, 2018

Pull Request Check List

This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once, it will save you unnecessary review cycles!

  • Added tests for changed code.
  • New features have been added to our Hypothesis testing strategy. ** n/a,I think**
  • Changes or additions to public APIs are reflected in our type stubs (files ending in .pyi).
  • Updated documentation for changed code.
  • Documentation in .rst files is written using semantic newlines.
  • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives.
  • Changes (and possible deprecations) have news fragments in changelog.d.

Description

This adds a cache_hash flag to @attrs which causes the hash code to be computed once and stored on the object. Setting this flag to true is allowed only if attrs is set to generate both the __hash__ and __init__ methods for a class.

This closes issues #423.

@gabbard
Copy link
Member Author

@gabbard gabbard commented Aug 19, 2018

@hynek : I believe this is now ready for review

Copy link
Member

@hynek hynek left a comment

For a PR of this size this looks quite excellent! I’ve added a bunch of nits, but this should be mergeable very soon!

raise TypeError(
"Invalid value for cache_hash. To use hash caching,"
" hashing must be either explicitly or implicitly "
"enabled"

This comment has been minimized.

raise TypeError(
"Invalid value for cache_hash. To use hash caching,"
" hashing must be either explicitly or implicitly "
"enabled"

This comment has been minimized.

if cache_hash:
raise TypeError(
"Invalid value for cache_hash. To use hash caching,"
" init must be True"

This comment has been minimized.

@@ -283,8 +293,34 @@ def test_enforces_type(self):

assert exc_args == e.value.args

@given(booleans())
def test_hash_attribute(self, slots):
def test_enforce_no_cache_hash_without_hash(self):

This comment has been minimized.

@@ -0,0 +1 @@
Added ``cache_hash`` option which causes the hash code to be computed once and stored on the object.

This comment has been minimized.

docs/hashing.rst Outdated

Some objects have hash codes which are expensive to compute.
If such objects are to be stored in hash-based collections, it can be useful to compute the hash codes only once and then store the result on the object to make future hash code requests fast.
To enable caching of hash codes, specify ``cache_hash=True``.

This comment has been minimized.

docs/hashing.rst Outdated
To enable caching of hash codes, specify ``cache_hash=True``.
This may only be done if ``attrs`` is already generating a hash function for the object.
If the hash code is cached, no field involved in hash code computation may be mutated after construction.
It is strongly recommended that classes with cached hashcodes be ``frozen``

This comment has been minimized.

This comment has been minimized.

docs/init.rst Outdated
@@ -348,6 +348,7 @@ If you need to set attributes on a frozen class, you'll have to resort to the :r
>>> Frozen(1)
Frozen(x=1, y=2)
Note that you may not access the hash code of the object in ``__attrs_post__init__`` if ``cache_hash=True``

This comment has been minimized.


if cache_hash:
method_lines.append(tab + "if self.%s is None:" % _hash_cache_field)
# we use __setattr__ all the time so we don't need to special-case for

This comment has been minimized.

This comment has been minimized.

)
assert exc_args == e.value.args

def test_enforce_no_cached_hash_without_init(self):

This comment has been minimized.

@gabbard
Copy link
Member Author

@gabbard gabbard commented Aug 19, 2018

@hynek : Ready for re-review. For all the trivial changes (periods, adding phrases to docs, docstrings, etc.) I made the suggested changes. In the two more complex cases, I commented on your comments to highlight my changes.

hynek
hynek approved these changes Aug 20, 2018
@hynek hynek merged commit 3e0ecbd into python-attrs:master Aug 20, 2018
3 checks passed
@hynek
Copy link
Member

@hynek hynek commented Aug 20, 2018

Great work, thank you!

@gabbard
Copy link
Member Author

@gabbard gabbard commented Aug 20, 2018

@hynek : no problem, thanks for reviewing and merging!

Do you happen to know when the next release is likely to be? My team would really like to use this feature and the new kw-only args support.

@gabbard gabbard deleted the 423-cache-hash branch Aug 20, 2018
@hynek
Copy link
Member

@hynek hynek commented Aug 21, 2018

Yeah it’s overdue, isn’t it. I want to fix #429 and #427 since both look like bugs and then release. So I hope this week if I find people to review my fixes.

@gabbard
Copy link
Member Author

@gabbard gabbard commented Aug 21, 2018

@hynek : Great, thank you!

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

Successfully merging this pull request may close these issues.

None yet

2 participants