isi-vista / immutablecollections

A library for immutable collections, in the spirit of Guava's Immutable Collections.
MIT License
3 stars 2 forks source link

ImmutableCollections should have serialized forms which are independent of implementation #27

Closed gabbard closed 5 years ago

gabbard commented 5 years ago
gabbard commented 5 years ago

@nicomitchell : you'll need to override __setstate__ and __getstate__ to control the serialized form. Don't bother with ImmutableList because it is going to die soon. For ImmutableSet, you can just use the ordering tuple as the serialized form. For the multi-dicts, use a tuple of key-value pairs.

berquist commented 5 years ago

Thought I was misunderstanding pickling, but there is in fact a blocker: https://github.com/python-attrs/attrs/issues/512 (with possible PR https://github.com/python-attrs/attrs/pull/513).

gabbard commented 5 years ago

When first implementing these we used attrs for convenience; is it buying us anything at this point or could we easily drop it? IIRC the main thing we get is the frozen=True behavior, which we could just copy.

berquist commented 5 years ago

Writing slots doesn't seem bad. Is making a frozen class as simple as https://github.com/python-attrs/attrs/blob/c2a9dd8e113a0dc72f86490e330f25bc0111971a/src/attr/_make.py#L478? Not like it's hard to get around it if you really want to...

from attr import attrs

class C:
    def __init__(self, val):
        self.val = val

@attrs(auto_attribs=True, frozen=False)
class A:
    m: C

@attrs(auto_attribs=True, frozen=True)
class Af:
    m: C

if __name__ == "__main__":
    c1 = C(1)
    c2 = C(2)
    a = A(c1)
    af = Af(c2)
    assert a.m.val == 1
    assert af.m.val == 2
    a.m.val = 10
    af.m.val = 20
    assert a.m.val == 10
    assert af.m.val == 20
gabbard commented 5 years ago

Yep, that simple.

You can definitely get around frozen if you want to but it prevents getting around it inadvertently. If the user wants to intentionally shoot themselves in the foot, it's their own fault.

berquist commented 5 years ago

In that case it might be worthwhile, since the validation and factory functions should be similarly thin. This would only be for immutablecollections and not other repositories/codebases, right?

gabbard commented 5 years ago

Yes - attrs is in general very useful; we only want to remove it here.

berquist commented 5 years ago

Now that https://github.com/isi-vista/immutablecollections/pull/38 is done, the serialized form can actually be changed.

berquist commented 5 years ago

@nicomitchell I'm working on this now, so we don't have duplicated effort.