ericvsmith / dataclasses

Apache License 2.0
587 stars 53 forks source link

Supporting immutable instances #7

Closed ericvsmith closed 7 years ago

ericvsmith commented 7 years ago

attrs has a frozen=True decorator parameter that causes all instances to be read-only. It does this by adding a setattr to the class which disallows overwriting attributes.

I like this feature, and I assume we should do the same thing. If anyone disagrees, please discuss here.

gvanrossum commented 7 years ago

I like it too. Does this automatically add a __hash__? What about sub-objects that have mutable attributes, or sub-objects that are e.g. lists or dicts?

ericvsmith commented 7 years ago

Yes, attrs (and my proposed code) would add a __hash__.

The way attrs and my code work is that they look through the current class and all base classes (in reverse MRO order) and find the fields. In attrs case, a field is something with a default value of attr.ib(), in my case it's something in __attributes__ (that is, it's a member variable annotated with a type). The generated __hash__ for the current class would include the hash of all of the fields in this and all subclasses. Non-fields are ignored in my case, and I assume for attrs, too.

So:

>>> @dataclass(hash=True)
... class B:
...   i: int
...
>>> @dataclass(hash=True)
... class C(B):
...   j: int
...
>>> hash(B(10)) == hash((10,))
True
>>> hash(C(1,2)) == hash((1,2))
True

The generated __hash__ for C would be return hash((self.i,self.j)).

Without frozen=True, this is how my code currently works:

>>> @dataclass(hash=True)
... class C:
...   i: int = 0
...
>>> hash(C())
-1660481949
>>> hash(C()) == hash((0,))
True
>>> hash(C([]))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<string>", line 2, in __hash__
TypeError: unhashable type: 'list'

attrs has the following rules:

If hash=None (default), the __hash__ method is generated according how cmp and frozen are set.

I think these are reasonable rules.

gvanrossum commented 7 years ago

Agreed.

ericvsmith commented 7 years ago

I've implemented this, except for the interactions between frozen, hash, and cmp. I'll do that shortly.

ilevkivskyi commented 7 years ago

@ericvsmith Just a minor note, you could mention an issue in PR description (or commit message like Fixes #7) this way we could easily track relations between issues and commits (also GitHub will close the issue automatically).

ericvsmith commented 7 years ago

Yeah, I apologize for that. I've not wanted to create dozens of issues for little changes, and I'm using this repo to communicate between the two machines I'm doing development on. I'll get more disciplined as I start writing the PEP. Thanks for the note, though.