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.

Sign up for GitHub

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

Jump to bottom

Fix/slot hashes #205

Closed
wants to merge 5 commits into from
Closed

Fix/slot hashes #205

wants to merge 5 commits into from

Conversation

Tinche
Copy link
Member

@Tinche Tinche commented Jun 6, 2017

This is ready for review I think.

@Tinche Tinche requested a review from hynek June 6, 2017 17:35
@codecov
Copy link

codecov bot commented Jun 6, 2017 •
edited
Loading

Codecov Report

Merging #205 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #205   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           9      9           
  Lines         589    592    +3     
  Branches      128    129    +1     
=====================================
+ Hits          589    592    +3
Impacted Files Coverage Δ
src/attr/_make.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24815d7...340e1a4. Read the comment docs.

@hynek hynek mentioned this pull request Jun 7, 2017
Open
@hynek hynek modified the milestone: 17.3 Jun 7, 2017
hynek
hynek requested changes Jun 7, 2017
View reviewed changes
Copy link
Member

@hynek hynek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly OK to me, just please make those small fixes. Also: what would we do without hypothesis…

src/attr/_make.py
@@ -368,6 +368,8 @@ def wrap(cls):
cls = _add_pickle(cls)
if slots is True:
cls_dict = dict(cls.__dict__)
was_unhashable = ('__hash__' in cls_dict and

This comment was marked as spam.

Sign in to view

src/attr/_make.py
@@ -376,6 +378,14 @@ def wrap(cls):

qualname = getattr(cls, "__qualname__", None)
cls = type(cls)(cls.__name__, cls.__bases__, cls_dict)

if (not was_unhashable and hash is False and

This comment was marked as spam.

Sign in to view

tests/test_hash.py


@given(slots=booleans(), frozen=booleans(), cmp=booleans())
def test_no_subclass_no_hash(slots, frozen, cmp):

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@hynek
Copy link
Member

hynek commented Jun 7, 2017

Oh and add the PR# to the changelog please.

@Tinche
Copy link
Member Author

Tinche commented Jun 8, 2017

I'll get around to this, just haven't had the time.

@hynek
Copy link
Member

hynek commented Jun 9, 2017

Well, given Nathaniel's comments I don't even know what to do anymore. :/ seems like we're digging ourselves deeper.

@hynek
Copy link
Member

hynek commented Jul 29, 2017

So I’m closing this and we’ll go the other way: break it on non-slots too eventually 🎉 so the behavior is consistent with Python’s.

@hynek hynek closed this Jul 29, 2017
@Tinche Tinche deleted the fix/slot-hashes branch August 1, 2017 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hynek hynek hynek requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
17.3
Development

Successfully merging this pull request may close these issues.
2 participants
@Tinche @hynek