__module__ should be set on functions returned by public_factory?

Issue #3218 resolved
Steven Winfield created an issue

This would be more in line with what functools.wraps() does, and code introspection tools could then determine where the returned function is supposed to live.

For example, if I do this:

from sqlalchemy.orm import relationship

...then sphinx (in particular) will generate documentation for relationship as if it were defined in the module containing the import.

I think the change is a one-liner - sqlalchemy/util/langhelpers.py before the return statement of public_factory (line 154):

decorated.__module__ = "sqlalchemy" + location.rsplit(".", 1)[0]

Comments (9)

  1. Mike Bayer repo owner

    also maybe we can just use functools.wraps() directly in public_factory. not sure it there was a reason i didn't use that.

  2. Mike Bayer repo owner

    OK so if I do that:

    diff --git a/lib/sqlalchemy/util/langhelpers.py b/lib/sqlalchemy/util/langhelpers.py
    index 9536978..1f2a1cc 100644
    --- a/lib/sqlalchemy/util/langhelpers.py
    +++ b/lib/sqlalchemy/util/langhelpers.py
    @@ -154,12 +154,8 @@ def %(name)s(%(args)s):
         env = {'cls': callable_, 'symbol': symbol}
         exec(code, env)
         decorated = env[location_name]
    -    decorated.__doc__ = fn.__doc__
    -    if compat.py2k or hasattr(fn, '__func__'):
    -        fn.__func__.__doc__ = doc
    -    else:
    -        fn.__doc__ = doc
    -    return decorated
    +    decorated.__wrapped__ = fn
    +    return update_wrapper(decorated, fn)
    
    
     class PluginLoader(object):
    

    which is what we do in decorator(), then I get what I don't want:

    #!
    
    >>> from sqlalchemy.sql import expression
    >>> expression.and_.__module__
    'sqlalchemy.sql.elements'
    >>> expression.desc.__module__
    'sqlalchemy.sql.elements'
    >>> 
    

    that is, I'm trying to have code spread out among "implementation" modules but that's not where I want people to look for it.

    I'm trying to do this:

        decorated = update_wrapper(
            decorated, fn, ('__doc__',), ('__module__', '__dict__'))
    

    and it's not working, it's throwing an error. Any clues appreciated else ill try to find time to look at it.

  3. Steven Winfield reporter

    With regards to how sphinx can be working on the sqlalchemy source now: From the sphinx source (sphinx.ext.autodoc, lines 411-429), it looks like sphinx will guess that an object was defined wherever it was found if __module__ is None, and will merrily create documentation for that object as if it were in that module (lines 423-424 are basically a hack to generate docs for everything)

    My guess is that since sqlalchemy's source doesn't do from sqlalchemy.orm import relationship anywhere, sphinx only creates one copy of relationship()'s docs - for the module where it is defined.

    I didn't use functools.wraps()/update_wrapper() for the same reasons that you're finding - you want __module__ to be set to the module containing the callsite of public_factory(), rather than copying the value from public_factory()s first argument. I think the only options are some grotty stack inspection inside public_factory() to see where it was called from or re-use the location argument as in the original post. I've tested that it works as expected:

    >>> from sqlalchemy.orm import relationship
    >>> import inspect
    >>> inspect.getmodule(relationship)
    <module 'sqlalchemy.orm' from 'c:\Python27\lib\site-packages\sqlalchemy\orm\__init__.pyc'>
    
  4. Mike Bayer repo owner

    OK update_wrapper() seems to claim you can customize this but at the moment it may not be worth fighting with.

  5. Steven Winfield reporter

    Thanks for your time with this.

    FWIW I think that by passing __module__ in the tuple as the last arg you're telling update_wrapper to call

    decorated.__module__.update(fn.__module__)
    

    ...which won't work because __module__ is a string/None and so doesn't have an update method.

    I don't think update_wrappers customisability stretches further than copying stuff from fn to decorated, which isn't what we want here.

  6. Mike Bayer repo owner
    • The __module__ attribute is now set for all those SQL and ORM functions that are derived as "public factory" symbols, which should assist with documentation tools being able to report on the target module. fixes #3218

    → <<cset fb09ad7551cf>>

  7. Log in to comment