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

Fix hashing behavior #142

Merged
merged 4 commits into from Feb 19, 2017
Merged

Fix hashing behavior #142

merged 4 commits into from Feb 19, 2017

Conversation

hynek
Copy link
Member

@hynek hynek commented Feb 4, 2017

Our previous default behavior was incorrect. Adding __hash__ has to be deliberate action and only makes sense for immutable classes.

Also set the doc theme to alabaster because the RTD one looks weird if a parameter doc has multiple paragraphs.

cc @njsmith @glyph @Tinche

@hynek hynek added this to the 17.1 milestone Feb 4, 2017
@codecov-io
Copy link

codecov-io commented Feb 4, 2017

Codecov Report

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

@@          Coverage Diff          @@
##           master   #142   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           8      8           
  Lines         544    551    +7     
  Branches      119    122    +3     
=====================================
+ Hits          544    551    +7
Impacted Files Coverage Δ
src/attr/init.py 100% <ø> (ø)
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 0563f44...ef30deb. Read the comment docs.

@Tinche
Copy link
Member

Tinche commented Feb 4, 2017

I'm in favor. I think this will be a clear improvement for 95% of the usage out there.

I think we could make the behavior smarter in the default case but honestly I don't know if it's worth the complexity. For example, we can detect if a custom __hash__ method has been written on the class we're decorating and if so, default to doing nothing even if @attr.s(hash=None). (Or raise an exception and ask the user to clarify.)
Also an idea floated around was to generate __hash__ for frozen classes by default. But I've used __hash__ so rarely I don't really have a strong opinion.

@hynek
Copy link
Member Author

hynek commented Feb 4, 2017

Trying to be smarter tends to be recipe for future disaster in my experience. :)

I think we should offer explicit APIs for certain archetypes but not try to guess what people want when they set certain flags…

@glyph
Copy link
Contributor

glyph commented Feb 7, 2017

An observation about compatibility: if the attr.ib list included a a mutable object (say, a dict or a list) is present in the list of hashed attributes, you'd already get an exception.

If we don't add a __hash__, that takes this to a silent id-based success, but this in fact adds a __hash__, just one that raises an exception.

This is "incompatible" in that code which relies on hash=True and is implicitly immutable will stop working, but the fix is pretty simple.

@hynek
Copy link
Member Author

hynek commented Feb 11, 2017

I have updated it according to @njsmith & @ambv ’s wishes.

The reason why I changed my mind is that if we break b/w compatibility to fix bad behavior, we should do it properly and make it worth it.

Please feed all the back.

@hynek hynek force-pushed the fix-hash branch 2 times, most recently from f039f42 to 0774042 Compare February 12, 2017 07:51
Our previous default behavior was incorrect.  Adding __hash__ has to be
deliberate action and only makes sense for immutable classes.

By default, `hash` now mirrors `cmp` which the correct behavior per spec.

Fixes #136.
@hynek
Copy link
Member Author

hynek commented Feb 16, 2017

cough Nathaniel?

@njsmith
Copy link

njsmith commented Feb 16, 2017

It looks like the logic that's implemented here is actually different from my suggestion? Specifically, in the case where if cmp=False (i.e., we're inheriting the superclass __eq__, if any), then I think we should also act like hash=False (i.e., also inherit the superclass __hash__, if any). Is that intentional, or...?

@hynek
Copy link
Member Author

hynek commented Feb 18, 2017

No, I just happen too be too dumb to program a computer properly. :(

It would be kind if you could have another look…

@njsmith
Copy link

njsmith commented Feb 19, 2017

I can't say I've read through every line with careful attention, but LGTM

@hynek hynek merged commit f560500 into master Feb 19, 2017
@hynek
Copy link
Member Author

hynek commented Feb 19, 2017

Thank you for your patience. ❤️

@njsmith
Copy link

njsmith commented Feb 19, 2017

Thank you for all your work!

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

Successfully merging this pull request may close these issues.

None yet

5 participants