zeekay / decorum

Python decorator helper library.
MIT License
14 stars 2 forks source link

Decorum's magic 'self._wrapped' should not be part of 'wraps' #28

Closed benoitbryon closed 9 years ago

benoitbryon commented 9 years ago

As of Decorum version 1.0.0, special attribute Decorum._wrapped is assigned within wraps(). See https://github.com/zeekay/decorum/blob/1.0.0/decorum/decorum.py#L57

It works quite well. But it means users MUST call super().wraps() if they override wraps() method, else the _wrapped attribute would be broken.

I wonder if we could move the setup of self._wrapped within both __init__() and __call__(), so that users who don't call super().wraps() just miss the assigned feature.

In general, I like the idea that Decorum's "magic" lives within __init__() and __call__(), whereas methods such as init(), wraps() and call() are simple to understand and override. Then I think self._wrapped is part of Decorum's magic, i.e. users should not have to bother about it in most cases.

zeekay commented 9 years ago

I agree.

benoitbryon commented 9 years ago

Oooops, I started working on such a refactor, and I'm not sure it is a good idea... What happens if, as a user, I want to change the wrapped function?

In current 1.0.0 version:

class my_decorator(Decorum):
    def wraps(self, f):
        new_function = lambda: True
        return super(my_decorator, self).wraps(new_function)

Calling super() makes sense. And we don't have to bother with Decorum's internal self._wrapped actually :)

If we move self._wrapped assignment in __init__() and __call__(), the example above will change like this:

class my_decorator(Decorum):
    def wraps(self, f):
        new_function = lambda: True
        self._wrapped = new_function
        return super(my_decorator, self).wraps(self._wrapped)  # Optional, but still recommended.

Here, we do have to know and care about self._wrapped, which is exactly what I wanted to avoid :(

zeekay commented 9 years ago

Yeah that makes sense.

benoitbryon commented 9 years ago

I propose closing this issue as wontfix. And if we have a better idea later, let's reopen or create a new ticket ;)

benoitbryon commented 9 years ago

By the way, thank you @zeekay for being so responsive!

zeekay commented 9 years ago

Haha, no worries, I'm always in a terminal ;)