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

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.

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.

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…

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.

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.

cough Nathaniel?

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...?

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

It would be kind if you could have another look…

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

Thank you for your patience. ❤️

Thank you for all your work!