Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Venusian should scan superclass for attributes #11

Closed
rbu opened this Issue Apr 3, 2012 · 6 comments

Comments

Projects
None yet
3 participants

rbu commented Apr 3, 2012

When decorating methods in a super class and a sub class, venusian will not execute callbacks from a superclass in a subclass. For example:

import venusian

def jsonify(wrapped):
    def callback(scanner, name, ob):
        print wrapped.func_name
    venusian.attach(wrapped, callback)
    return wrapped

class SuperClass(object):
    @jsonify
    def froz_fnord(self): pass

class SubClass(SuperClass):
    @jsonify
    def bar_baz(self): pass

class module(object):
    sub = SubClass() # in this example, this could also be sub = SubClass

venusian.Scanner().scan(module)

My expected output would be:
froz_fnord
bar_baz

The actual output is:
bar_baz

As a workaround, if the scanned module contains an instance of SubClass (as above), the attribute dicts can be merged as follows:

class SubClass(SuperClass):
    @jsonify
    def bar_baz(self): pass

    def __getattribute__(self, name):
        if name == venusian.ATTACH_ATTR:
            def merge_dict_from_superclass(merge_into, merge_from):
                for key, values in merge_from.iteritems():
                    merge_into.setdefault(key, []).extend(values)

            classes = list(self.__class__.__bases__) + [self.__class__]
            attached_attributes = venusian.Categories(None)
            for _class in classes:
                try:
                    more_attributes = object.__getattribute__(_class, venusian.ATTACH_ATTR)
                    merge_dict_from_superclass(attached_attributes, more_attributes)
                except AttributeError:
                    continue
            return attached_attributes
        return object.__getattribute__(self, name)

This produces the desired output.

dwt commented Apr 3, 2012

Thats just what I was missing in the API too!

Can you fix this please?

Owner

mcdonc commented Apr 4, 2012

AFAICT, this was argued the other way in #1 . I think subclasses should need to support this behavior explicitly somehow, instead of implicitly.

rbu commented Apr 5, 2012

I completely agree on making this explicit.

Right now, this behavior is inconsistent when decorating methods within a class (as in the above example) versus decorating a class (as demonstrated in venusian's unit test test_decorations_arent_inherited). When decorating a method in a parent class, the subclass will inherit the decoration if it does not use any other venusian decorator:
In the above example, if I change the subclass to be

class SubClass(SuperClass):
    def bar_baz(self): pass

the output will be:
froz_fnord

rbu commented Apr 5, 2012

I'm not sure what a complete solution here is. From what I understand, when mixing decorated methods and inheritance, in the case seen above, the subclass does not inherit the venusian attribute because in the venusian.attach function, the setattr:

scope, module, f_locals, f_globals, codeinfo = getFrameInfo(frame)
if scope == 'class':
    # we're in the midst of a class statement
    categories = f_locals.setdefault(ATTACH_ATTR, Categories(None))

will create an entry in the class locals, i.e. the class dict, shadowing the attribute from the super class from the parent class. I have implemented a unit test and a partial fix for this, as seen in:
https://github.com/rbu/venusian/compare/issue-11

Unfortunately, this fix is not complete as it does cover the case where the scanner is run on a module that contains instances of a class with a decorated method, see the failing test case in ae45d32.

An idea to solve the inconsistency would be to

  • store the scope variable (class/module) in the Categories dict
  • in the scan method, when finding a dict with scope=='class', and isclass(ob) == False, verify ATTACH_ATTR in ob.__class__.__dict__

For what it's worth, none of the above addresses the problem at hand in this bug report, which is making the idea of 'inherit decorations from superclass' configurable. How about adding a keyword argument to attach that (a) changes behavior of the attach method in the scope!='class' case and (b) is stored in the categories dict so that scanning can walk the inheritance chain in case of scope=='class'.

@mcdonc mcdonc added a commit that referenced this issue Jun 8, 2013

@mcdonc mcdonc - Fix bug where otherwise undecorated subclass of a superclass that had
  venusian decorators on it would inherit its superclass' decorations.
  Venusian decorators should have never been inherited implicitly.  See
  #11 (comment)

Ref #11
11857ea
Owner

mcdonc commented Jun 8, 2013

I believe I've now reproduced and fixed the bug that was reported in #11 (comment)

I've also added "lift" and "onlyliftedfrom" decorators in the master, which, respectively, tell a subclass to inherit decorations from its superclasses, and which tells a class that its decorations are only for inheriting. Maybe you could take a look at those decorators and see if they work for you.

Owner

mcdonc commented Jul 14, 2014

Closing; if lifted and onlyliftedfrom dont work for you, please reopen with specifics.

@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