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

Add a convenience flag for upcalling on Exception subclasses #368

Closed
habnabit opened this issue Apr 11, 2018 · 7 comments
Closed

Add a convenience flag for upcalling on Exception subclasses #368

habnabit opened this issue Apr 11, 2018 · 7 comments
Labels
Milestone

Comments

@habnabit
Copy link

Currently, these tests both fail on 2.7 and only the latter fails on 3.6.

@attr.s
class E1(Exception):
    x = attr.ib()
    y = attr.ib()


def test_exception_init_called():
    e = E1(1, 2)
    assert e.args == (e.x, e.y) == (1, 2)


def test_exception_init_called_even_with_kw():
    e = E1(y=3, x=4)
    assert e.args == (e.x, e.y) == (4, 3)

If attrs is overriding __init__ then this makes sense; args is filled in by __init__ in 2.x and __new__ in 3.x, but only pulling from the *args and never **kwargs. I discussed this a bit with @markrwilliams but I'm not sure what the best way to handle an attrs-ified exception would be. Maybe it's okay to consider args vestigal.

Mostly I'm raising this on the issue tracker because there are some references to using attrs for exceptions, but only indirectly. It'd be nice to have some documentation on a suggested approach either way.

@wsanchez
Copy link

Is test_exception_init_called_even_with_kw a valid test?

The args attribute on Exception is documented as "The tuple of arguments given to the exception constructor."

And Exception itself doesn't accept kwargs. So I think the behavior in the case of kwargs to init with respect to what goes into the args attribute is… undefined. So I'd have no expectation of that test passing on exception classes generically.

@habnabit
Copy link
Author

I included it because of the usual attrs isomorphism:

class E1(Exception):
    def __init__(self, x, y):
        super(E1, self).__init__(x, y)
        self.x = x
        self.y = y

It might not abide strictly by the documentation of Exception that you'd end up with e.args == (e.x, e.y) but I wasn't sure if a case could be made since this is how one would write the class without attrs.

@wsanchez
Copy link

wsanchez commented Apr 11, 2018

I guess what I'm saying is that it's not clear to me that when one calls the super init, that including the kwargs is a common practice.

That said, I find the args attribute's design to be severely lacking in clarity, basically a "tuple of whatever rando stuff the caller threw in here", so I'm rather sympathetic to the argument that it is vestigal.

The only reason to care about it as far as I'm aware is that it effects str(), and that's true even when using attrs, which leaves something to be desired:

>>> str(C(1,2))
'C(x=1, y=2)'
>>> str(C(x=1,y=2))
'C(x=1, y=2)'
>>> str(E1(1,2))
'(1, 2)'
>>> str(E1(x=1,y=2))
''

Here E1 is defined as in your example, and C is the same as E1, but without inheriting from Exception.

@rouge8
Copy link
Contributor

rouge8 commented May 2, 2018

After running into #217 I came up with an @attrs_exception decorator that passes both of your tests on Python 2.7 (and probably 3.6):

import inspect

import attr


def attrs_exception(maybe_cls=None, *args, **kwargs):
    r"""
    A class decorator to eliminate boilerplate when creating Exceptions using
    :func:`attr.s`.

    Adds ``__str__`` (because Exceptions don't use ``__repr__``) and an
    ``__attrs_post_init__`` that calls ``Exception.__init__`` with the class's
    :func:`attr.ib`\ s. Without this, instances cannot be deepcopied on Python 2.

    ref: https:/python-attrs/attrs/issues/217

    :param args: additional positional arguments to be passed to :func:`attr.s`
    :param kwargs: additional keyword arguments to be passed to :func:`attr.s`
    """
    def wrap(cls):
        def _attrs_post_init__(obj):
            Exception.__init__(obj, *attr.astuple(obj))

        existing_post_init = getattr(cls, '__attrs_post_init__', None)
        if (
            existing_post_init
            and (
                inspect.getsource(existing_post_init)
                != inspect.getsource(_attrs_post_init__)
            )
        ):
            raise TypeError(
                "'%s' already has an '__attrs_post_init__' method" % (cls,),
            )
        if not issubclass(cls, Exception):
            raise TypeError(
                "'%s' is not a subclass of Exception! Use 'attr.s' instead" % (cls,),
            )
        cls.__attrs_post_init__ = _attrs_post_init__

        kwargs['str'] = True
        kwargs['slots'] = True
        return attr.attributes(cls, *args, **kwargs)

    # maybe_cls depends on the usage of the decorator. It's a class if it's
    # used as @attrs_exception but None if it's used as @attrs_exception()
    if maybe_cls is None:
        return wrap
    else:
        return wrap(maybe_cls)

If anyone is interested, I can probably put that (and its accompanying tests) on PyPI.

@hynek
Copy link
Member

hynek commented Jun 16, 2018

So, what we want here is basically:

  1. @attr.s(str=True)
  2. All fields additionally put into self.args?
  3. Exceptions usually can’t be compared – so maybe also cmp=False?

I’d be sympathetic to add something like @attr.exc since I use it quite often myself.

@hynek hynek added the Feature label Jun 16, 2018
twm added a commit to twm/yarrharr that referenced this issue Jun 25, 2018
See python-attrs/attrs#368

Maybe attrs should do this automatically when subclassing BaseException?
@glyph
Copy link
Contributor

glyph commented Oct 15, 2018

Just hit this myself, blindly assuming Exceptions would work - @attr.exc (@attr.err?) would be very handy.

@hynek hynek added this to the import attrs milestone Feb 9, 2019
hynek added a commit that referenced this issue Feb 10, 2019
hynek added a commit that referenced this issue Feb 10, 2019
hynek added a commit that referenced this issue Feb 10, 2019
hynek added a commit that referenced this issue Feb 10, 2019
@hynek
Copy link
Member

hynek commented Feb 10, 2019

I took a stab at this in #500.

Please provide feedback so we can close this gaping hole.

hynek added a commit that referenced this issue Feb 24, 2019
hynek added a commit that referenced this issue Feb 25, 2019
hynek added a commit that referenced this issue Feb 25, 2019
* Implement first class exception support

Fixes #368

* Ensure single-attrib classes work too

cf #500 (review)

* Call into BaseException to initialiaze self.args

* Leave __str__ alone since we upcall

Based on Python pizza hallway feedback by @ambv.

* remove stray stage

* nope
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

5 participants