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

attrs-decorated classes don't play nicely with sibling __init__ methods #58

Closed
zenbot opened this issue Aug 15, 2016 · 6 comments
Closed
Labels

Comments

@zenbot
Copy link

zenbot commented Aug 15, 2016

attrs-decorated classes can produce unexpected behaviour when they exist in an inheritance tree with non-attrs-decorated classes that define their own __init__ methods, because those __init__ methods aren't called. This dumb test exercises this:

def test_init_with_multiple_inheritance():
    import attr

    @attr.s
    class A(object):
        a = attr.ib(default=42)

    class B(object):
        def __init__(self):
            super(B, self).__init__()
            self.b = 3

    class C(A, B):
        pass

    c = C()
    assert c.a == 42
    assert c.b == 3

The second assertion currently fails with an AttributeError.

In ordinary Python code this would be addressed with a super call in A.__init__, but I'm not sure if adding a super to the __init__ returned from _attrs_to_script would achieve the same result without breaking a bunch of other cases.

(I apologise for not digging further & submitting a patch; the code in _make.py is a little too terrifying to get stuck into a Monday morning!)

@hynek
Copy link
Member

hynek commented Aug 15, 2016

The problem is, that attrs has no way to know, how to call super (i.e. with what arguments).

I believe this could be solved by the suggested solutions to #25 (IOW: support for a custom __init__ hook)?

Until then you'll have to write your own __init__ sadly.

@Insoleet
Copy link
Contributor

Insoleet commented Aug 16, 2016

I have a suggestion : use something like PyQt5 does ( http://pyqt.sourceforge.net/Docs/PyQt5/multiinheritance.html )

For example, the following code :

    @attr.s
    class A(object):
        a = attr.ib(default=42)

would generate something like this :

class A():
    def __init__(self, a=42, **kwds):
        super().__init__(**kwds)

        self.a = a

This way we could pass arguments to the super().__init__() method and safely call it using named parameters.

What do you think about it ? Are there any case which would not be covered by this behavior ?

EDIT : Here is a simple implementation which works fine for my case :

# the method is called by passing the cl.__base__ as the base parameter
def _attrs_to_script(attrs, base):
    """
    Return a valid Python script of an initializer for *attrs*.
    """

    # current code generating args does not change
    # [...]
    # I append the following :
    if base is not object:
        args.append("**kwargs")
        supercall = """
    frommodule = __import__('{module}', fromlist=[' '])
    supclass = getattr(frommodule, '{base}')
    super(supclass, self).__init__(**kwargs)""".format(base=base.__name__,
                                                           module=base.__module__)
    else:
        supercall = ""

    return """\
def __init__(self, {args}):
{supercall}
    {setters}
""".format(
        args=", ".join(args),
        supercall=supercall,
        setters="\n    ".join(lines) if lines else "pass",
    )

This works fine with a class such as the following :

from PyQt5.QtCore import QObject


@attr.s
class Foo(QObject):

    a = attr.ib()
    b = attr.ib()
    c = attr.ib()

Calling the constructor like this : foo = Foo(1, 2, 3, parent=None)
QObject.super(parent) is correctly called with None as a value.

I needed to import QObject in the generated script code because if I don't, I would get a NameError : name QObject not found. The empty fromlist is because of the following fact : http://stackoverflow.com/questions/2724260/why-does-pythons-import-require-fromlist

@hynek
Copy link
Member

hynek commented Aug 17, 2016

Honestly this looks like something with many edge cases and I explicitly do not want to have **kwargs in my __init__s because attrs is all about explicit APIs and zero-cost code generation. This approach adds quite a bit of runtime complexity whenever someone uses subclassing and muddles up the signatures of initializers..

I feel like this might be a great case for writing an attrs extension like @super_init that will implement exactly what you’re describing. attrs was always conceptualized for extensibility because of cases like this.

Another alternative might be once the aforementioned post-init-hook lands (hopefully 16.1) to add an explicit allow_kwargs option that lets you do your thing in __post_init__.

@Insoleet
Copy link
Contributor

Insoleet commented Aug 17, 2016

No problem, it was mostly a proof-of-concept to see what you would think about it. I understand your reasoning.
I'll try to realize an extension based on this concept. I'll need to modify the generated __init__ method. Is there any recommended way to do it ?

@hynek
Copy link
Member

hynek commented Aug 17, 2016

Not yet! If someone gives me a strong case + sane API I won’t regret in the future, I may make some innards public. But at this point you can only use the internal functions and play around with strings. And those can change quite radically, as seen in #60.

That’s why I suspect a hook-based system is better for everyone involved.

@glyph glyph mentioned this issue Aug 21, 2016
@hynek hynek added the Feature label Aug 21, 2016
@hynek
Copy link
Member

hynek commented Feb 4, 2017

I believe this is fixed by __attrs_post_init__! Closing for now but please let me know if there’s anything broken.

@hynek hynek closed this as completed Feb 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants