Patch: fix __module__ for instrumented attribute

Issue #1788 resolved
Anonymous created an issue

If {{{Foo}}} is a class and {{{Foo.bar}}} is an attribute of that class, it is reasonable to assume that {{{Foo.bar.module == Foo.module}}}.

Attached is a simple patch for this issue along with a test case. As it doesn't apply cleanly to sa05, and a backport would make me very happy, I have included a patch for both trunk and sa05.

For what it's worth, Sphinx is one tool that operates under the above assumption and this patch will fix an [issue with generating documentation for model classes](http://groups.google.com/group/sphinx-dev/browse_thread/thread/1be84427fad69529/d2f568d5ef9f306a).

Comments (9)

  1. Anonymous

    Just realized that the sa05 patch is probably bogus, I didn't use hg merge to make it but just applied the diff with "patch"... I've never worked with mercurial before so not sure how to do it right. Sorry about that.

  2. jek

    This sounds like a sphinx issue. The assumption that Class.module == Class.prop.module is spurious- it's not part of the descriptor protocol or Python class composition in general.

    For example, in the builtins, @Property doesn't set a module on its descriptor instance, classmethod(otherpackage.function) reports a module of otherpackage on the descriptor.

    Information on why this is change required in this instance is needed, plus ensuring that masking the module does not break source code reporting in pdb or other debuggers.

  3. Michael Bayer repo owner

    The change in e1c47d12bf871bf30a149c7fc167d5d22ddaa222 removes the generation of the "default" docstring by just setting doc to None if not otherwise present. I suppose I'm only using ..automodule and ..autoclass so far. I don't necessarily agree that __module__ should be on descriptors - the descriptor implementation does come from a different module and it feels wrong to conceal that.

  4. Anonymous

    Fair points, it looks like I'll have to do some more digging.

    For now I'd like to point out that {{{Table.attr.class}}} is still {{{InstrumentedAttribute}}}, {{{Table.attr.class.module}}} is still {{{sqlalchemy.orm.attributes}}}, and raising an exception in {{{InstrumentedAttribute}}} code still shows the correct source information after this change. So I'm not sure that you can say this change conceals anything.

    I'll test source code reporting in pdb when I find some time later on, and I'll also see if @Property also breaks Sphinx. If so, that would probably be a good indicator that this should indeed be fixed in Sphinx.

  5. Log in to comment