venusian attributes and inheritance #14

Closed
breathe opened this Issue Aug 14, 2012 · 5 comments

Comments

Projects
None yet
2 participants

breathe commented Aug 14, 2012

I don't think venusian callback attributes are handled correctly when used for class decorators ... The implementation seems inconsistent and its unclear to me how this design might actually be used ... With the current implementation I can't use class decorators on a superclass and expect the handler to be added to subclasses and neither can I reliably use the same decorator on a superclass and a subclass.

Using a class decorator on a superclass and at least one subclass means all super/subclasses share the venusian callbacks:

@decorate 
class SuperClass: 
 pass

@decorate 
class SubClass(SuperClass):
 pass

class OtherSubClass(SuperClass):
 pass

OtherSubClass.__venusian_callbacks__ == SubClass.__venusian_callbacks__ == SuperClass.__venusian_callbacks__

Using a class decorator on a superclass and no subclasses means only the superclass gets the callbacks:

@decorate 
class SuperClass: 
 pass

class SubClass(SuperClass):
 pass

class OtherSubClass(SuperClass):
 pass

Subclass.__venusian_callbacks__

raises attribute error. It seems to me there's no effective way to deliberately use venusian for class decoration ... I can't implement a design where a decorated class and all its subclasses are automatically decorated and found during a venusian scan -- and neither can I manually apply the same decoration to all the subclass objects as it results in many callbacks being registered in the shared attribute array of each sub and superclass ... I would argue that this is a bug but maybe I'm just not seeing an effective way to use this?

Thanks

Owner

mcdonc commented Aug 15, 2012

The stuff you mention above is by design, for better or worse. Causing subclasses to be found during a scan when a superclass has a decoration would cause all kinds of nightmarish scenarios for certain types of decorations. For example, if a superclass was decorated with a Pyramid "view_config" decorator, and a scan found the same decorations on a subclass, both the superclass and the subclass would be registered with identical view configuration values, which would cause conflicting configuration statements (the upshot being that the process wouldn't start, in Pyramid's case). I'd suggest that the desire to inherit venusian decorations is ill-conceived, and you would agree if you attempted to use such a feature in any sort of anger.

breathe commented Aug 15, 2012

I'm happy to concede that inheriting
Venusian attributes is not desirable -- in fact it's the behavior I was expecting! My point is that these attributes do seem to be inheritable in a non-desirable way ...

If a subclass and superclass both use Venusian class decorators than the superclass and all subclasses share the same set of venusian callbacks (including subclasses which do not have the Venusian class decorator) ... This must be a bug ...?

Sent from my iPhone

On Aug 15, 2012, at 2:46, Chris McDonough notifications@github.com wrote:

The stuff you mention above is by design, for better or worse. Causing subclasses to be found during a scan when a superclass has a decoration would cause all kinds of nightmarish scenarios for certain types of decorations. For example, if a superclass was decorated with a Pyramid "view_config" decorator, and a scan found the same decorations on a subclass, both the superclass and the subclass would be registered with identical view configuration values, which would cause conflicting configuration statements (the upshot being that the process wouldn't start, in Pyramid's case). I'd suggest that the desire to inherit venusian decorations is ill-conceived, and you would agree if you attempted to use such a feature in any sort of anger.


Reply to this email directly or view it on GitHub.

Owner

mcdonc commented Aug 15, 2012

Do you think you have time to create a test case in the form of a pull request to venusian here that demonstrates the issue? You can run the venusian tests by doing:

$ git clone git@github.com:breathe/venusian.git $ cd venusian $ virtualenv -p python2.7 env27 $ env27/bin/python setup.py dev # downloads testing deps too $ env27/bin/python setup.py nosetests

Adding a test is a matter of changing a file in the directory implied by https://github.com/Pylons/venusian/tree/master/venusian/tests

Once you've added one, it'd be nice to receive a pull request from you so I can put it in the master branch and address it if it needs addressing.

breathe commented Aug 16, 2012

I worked up a test case demonstrating the issue:

#15

Owner

mcdonc commented Jul 14, 2014

Closing in favor of further conversation on #15.

@mcdonc mcdonc closed this Jul 14, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment