Woah!  This is awesome, thanks for working on this.

Just a few comments, but they're pretty minor.  I'd be happy to merge
this as is, but I'll give you time to reply first.

Adi Roiban <email address hidden> writes:

> Adi Roiban has proposed merging lp:~adiroiban/pydoctor/1287458-objects.inv into lp:pydoctor.
>
> Requested reviews:
>   Michael Hudson-Doyle (mwhudson)
> Related bugs:
>   Bug #1287458 in pydoctor: "Generate Sphinx object inventory instead of pickle"
>   https://bugs.launchpad.net/pydoctor/+bug/1287458
>
> For more details, see:
> https://code.launchpad.net/~adiroiban/pydoctor/1287458-objects.inv/+merge/219116
>
> Project
> ------
>
> pydoctor pickle system is broken.
>
> Rather than fixing picker, the solution was to support sphinx inventory objects.inv
>
>
> Changes
> -------
>
> I have note removed pickle code, but created a separate ticket for that.. as I have no idea how pickle should be used.

pickle should not be used, ever.  I think I even knew this when I was
first writing pydoctor all those years ago, I don't know what I was
thinking.

> When html is generated it will alwasy generate an objects.inv file.
> I did not add any options, trying to keep things simple.
>
> Also file is hardcoded to objects.inv and there is no option to use a different file name.

That's also the way sphinx works right?  It assumes that there is a
objects.inv at $base_url/objects.inv?  If so, I agree there is no point
adding flexibility to pydoctor...

> I have added some tests and updated documentation.

Thanks :-)

> How to test
> -----------
>
> I tried this on my local computer with sphinx documentation. Syntax is documented in README.
>
> intersphinx plugin can be configured with local paths:
>
> intersphinx_mapping = {
>     'py2': ('http://docs.python.org/2.7', None),
>     'py3': ('http://docs.python.org/3.3', None),
>     'pydoctor': ('file:///home/adi/chevah/pydoctor/apidocs', None),
> }
>
> Please check that changes make sense and let me know if something needs changes.
> Thanks!
>
> -- 
> https://code.launchpad.net/~adiroiban/pydoctor/1287458-objects.inv/+merge/219116
> You are requested to review the proposed merge of lp:~adiroiban/pydoctor/1287458-objects.inv into lp:pydoctor.
> === modified file 'README.txt'
> --- README.txt	2013-05-29 03:28:59 +0000
> +++ README.txt	2014-05-11 11:23:52 +0000
> @@ -14,3 +14,33 @@
>  The default HTML generator requires Twisted.
>  
>  There are some more notes in the doc/ subdirectory.
> +
> +
> +Sphinx Integration
> +------------------
> +
> +HTML generator will also generate a Sphinx objects inventory using the
> +following mapping:
> +
> +* packages, modules -> py:mod:
> +* classes -> py:class:
> +* functions -> py:func:
> +* methods -> py:meth:
> +* attributes -> py:attr:
> +
> +Configure Sphinx intersphinx extension:
> +
> +    intersphinx_mapping = {
> +        'pydoctor': ('http://domain.tld/api', None),
> +    }
> +
> +Use external references::
> +
> +    :py:func:`External API <pydoctor:pydoctor.model.Documentable.reparent>`
> +
> +    :py:mod:`pydoctor:pydoctor`
> +    :py:mod:`pydoctor:pydoctor.model`
> +    :py:func:`pydoctor:pydoctor.driver.getparser`
> +    :py:class:`pydoctor:pydoctor.model.Documentable`
> +    :py:meth:`pydoctor:pydoctor.model.Documentable.reparent`
> +    :py:attr:`pydoctor:pydoctor.model.Documentable.kind`
>
> === modified file 'pydoctor/driver.py'
> --- pydoctor/driver.py	2013-05-29 22:51:44 +0000
> +++ pydoctor/driver.py	2014-05-11 11:23:52 +0000
> @@ -1,6 +1,7 @@
>  """The command-line parsing and entry point."""
>  
>  from pydoctor import model, zopeinterface
> +from pydoctor.sphinx import SphinxInventory
>  import sys, os
>  
>  def error(msg, *args):
> @@ -331,6 +332,8 @@
>                  'warning',
>                  'WARNING: guessing '+name+' for project name', thresh=-1)
>              system.guessedprojectname = name
> +        else:
> +            system.guessedprojectname = system.options.projectname

So this seems a bit strange ... I guess there should be a
system.projectname that gets set here and that should be referred to in
the places that currently check options.projectname and fall back to
guessedprojectname.  I can do this if you don't want to though :-)

>          # step 4: save the system, if desired
>  
> @@ -391,6 +394,17 @@
>                  f.close()
>                  system.options = options
>  
> +            # Generate Sphinx inventory.
> +            sphinx_inventory = SphinxInventory(
> +                logger=system.msg,
> +                project_name=system.guessedprojectname,
> +                )
> +            sphinx_inventory.generate(
> +                subjects=subjects,
> +                basepath=options.htmloutput,
> +                )
> +
> +
>          # Finally, if we should serve html, lets serve some html.
>          if options.server:
>              from pydoctor.server import (
>
> === added file 'pydoctor/sphinx.py'
> --- pydoctor/sphinx.py	1970-01-01 00:00:00 +0000
> +++ pydoctor/sphinx.py	2014-05-11 11:23:52 +0000
> @@ -0,0 +1,103 @@
> +"""
> +Support for Sphinx compatibility.
> +"""
> +from __future__ import absolute_import
> +import os
> +import zlib
> +
> +from pydoctor import model
> +
> +
> +class SphinxInventory(object):
> +    """
> +    Sphinx inventory handler.
> +    """
> +
> +    version = (2, 0)
> +
> +    def __init__(self, logger, project_name):
> +        self.project_name = project_name
> +        self.msg = logger
> +
> +    def generate(self, subjects, basepath):
> +        """
> +        Generate Sphinx objects inventory version 2 at `basepath`/objects.inv.
> +        """
> +        path = os.path.join(basepath, 'objects.inv')
> +        self.msg('sphinx', 'Generating objects inventory at %s' % (path,))
> +
> +        with self._openFileForWriting(path) as target:
> +            target.write(self._generateHeader())
> +            content = self._generateContent(subjects)
> +            target.write(zlib.compress(content))
> +
> +    def _openFileForWriting(self, path):
> +        """
> +        Helper for testing.
> +        """
> +        return open(path, 'w')
> +
> +    def _generateHeader(self):
> +        """
> +        Return header for project  with name.
> +        """
> +        version = [str(part) for part in self.version]
> +        return """# Sphinx inventory version 2
> +# Project: %s
> +# Version: %s
> +# The rest of this file is compressed with zlib.
> +""" % (self.project_name, '.'.join(version))
> +
> +    def _generateContent(self, subjects):
> +        """
> +        Write inventory for all `subjects`.
> +        """
> +        content = []
> +        for obj in subjects:
> +            content.append(self._generateLine(obj))
> +            content.append(self._generateContent(obj.orderedcontents))
> +
> +        return ''.join(content)
> +
> +    def _generateLine(self, obj):
> +        """
> +        Return inventory line for object.
> +
> +        name domain_name:type priority URL display_name
> +
> +        Domain name is always: py
> +        Priority is always: -1
> +        Display name is always: -
> +        """
> +        full_name = obj.fullName()
> +        display = '-'
> +        anchor = None
> +        if isinstance(obj, (model.Package, model.Module)):
> +            domainname = 'module'
> +            base_url = full_name
> +        elif isinstance(obj, model.Class):
> +            domainname = 'class'
> +            base_url = full_name
> +        elif isinstance(obj, model.Function):
> +            if obj.kind == 'Method':
> +                domainname = 'method'
> +            else:
> +                domainname = 'function'
> +            base_url = obj.parent.fullName()
> +            anchor = obj.name
> +        elif isinstance(obj, model.Attribute):
> +            domainname = 'attribute'
> +            base_url = obj.parent.fullName()
> +            anchor = obj.name
> +        else:
> +            domainname = 'obj'
> +            base_url = full_name
> +            self.msg('sphinx', "Unknown type for %s." % (full_name,))

This seems like it would be cleaner to do a mapping based on obj.kind I
think?

> +        base_url += '.html'
> +        if anchor:
> +            url = '%s#%s' % (base_url, anchor)
> +        else:
> +            url = base_url
> +
> +        return '%s py:%s -1 %s %s\n' % (full_name, domainname, url, display)
>
> === added file 'pydoctor/test/test_sphinx.py'
> --- pydoctor/test/test_sphinx.py	1970-01-01 00:00:00 +0000
> +++ pydoctor/test/test_sphinx.py	2014-05-11 11:23:52 +0000
> @@ -0,0 +1,201 @@
> +"""
> +Tests for Sphinx integration.
> +"""
> +from contextlib import closing
> +from StringIO import StringIO
> +
> +from pydoctor import model
> +from pydoctor.sphinx import SphinxInventory
> +
> +
> +class PersistentStringIO(StringIO):
> +    """
> +    A custom stringIO which keeps content after file is closed.
> +    """
> +    def close(self):
> +        """
> +        Close, but keep the memory buffer and seek position.
> +        """
> +        if not self.closed:
> +            self.closed = True
> +
> +    def getvalue(self):
> +        """
> +        Retrieve the entire contents of the "file" at any time even after
> +        the StringIO object's close() method is called.
> +        """
> +        if self.buflist:
> +            self.buf += ''.join(self.buflist)
> +            self.buflist = []
> +        return self.buf
> +
> +
> +def test_initialization():
> +    """
> +    Is initialized with logger and project name.
> +    """
> +    logger = object()
> +    name = object()
> +
> +    sut = SphinxInventory(logger=logger, project_name=name)
> +
> +    assert logger is sut.msg
> +    assert name is sut.project_name
> +
> +
> +def test_generate_empty_functional():
> +    """
> +    Functional test for index generation of empty API.
> +
> +    Header is plain text while content is compressed.
> +    """
> +    project_name = 'some-name'
> +    log = []
> +    logger = lambda part, message: log.append((part, message))
> +    sut = SphinxInventory(logger=logger, project_name=project_name)
> +    output = PersistentStringIO()
> +    sut._openFileForWriting = lambda path: closing(output)
> +
> +    sut.generate(subjects=[], basepath='base-path')
> +
> +    expected_log = [(
> +        'sphinx',
> +        'Generating objects inventory at base-path/objects.inv'
> +        )]
> +    assert expected_log == log
> +
> +    expected_ouput = """# Sphinx inventory version 2
> +# Project: some-name
> +# Version: 2.0
> +# The rest of this file is compressed with zlib.
> +x\x9c\x03\x00\x00\x00\x00\x01"""
> +    assert expected_ouput == output.getvalue()
> +
> +
> +def make_SphinxInventory():
> +    """
> +    Return a SphinxInventory.
> +    """
> +    return SphinxInventory(logger=object(), project_name='project_name')
> +
> +
> +def test_generateContent():
> +    """
> +    Return a string with inventory for all  targeted objects, recursive.
> +    """
> +    sut = make_SphinxInventory()
> +    system = model.System()
> +    root1 = model.Package(system, 'package1', 'docstring1')
> +    root2 = model.Package(system, 'package2', 'docstring2')
> +    child1 = model.Package(system, 'child1', 'docstring3', parent=root2)
> +    system.addObject(child1)
> +    subjects = [root1, root2]
> +
> +    result = sut._generateContent(subjects)
> +
> +    expected_result = (
> +        'package1 py:module -1 package1.html -\n'
> +        'package2 py:module -1 package2.html -\n'
> +        'package2.child1 py:module -1 package2.child1.html -\n'
> +        )
> +    assert expected_result == result
> +
> +
> +def test_generateLine_package():
> +    """
> +    Check inventory for package.
> +    """
> +    sut = make_SphinxInventory()
> +
> +    result = sut._generateLine(
> +        model.Package('ignore-system', 'package1', 'ignore-docstring'))
> +
> +    assert 'package1 py:module -1 package1.html -\n' == result
> +
> +
> +def test_generateLine_module():
> +    """
> +    Check inventory for module.
> +    """
> +    sut = make_SphinxInventory()
> +
> +    result = sut._generateLine(
> +        model.Module('ignore-system', 'module1', 'ignore-docstring'))
> +
> +    assert 'module1 py:module -1 module1.html -\n' == result
> +
> +
> +def test_generateLine_class():
> +    """
> +    Check inventory for class.
> +    """
> +    sut = make_SphinxInventory()
> +
> +    result = sut._generateLine(
> +        model.Class('ignore-system', 'class1', 'ignore-docstring'))
> +
> +    assert 'class1 py:class -1 class1.html -\n' == result
> +
> +
> +def test_generateLine_function():
> +    """
> +    Check inventory for function.
> +
> +    Functions are inside a module.
> +    """
> +    sut = make_SphinxInventory()
> +    parent = model.Module('ignore-system', 'module1', 'docstring')
> +
> +    result = sut._generateLine(
> +        model.Function('ignore-system', 'func1', 'ignore-docstring', parent))
> +
> +    assert 'module1.func1 py:function -1 module1.html#func1 -\n' == result
> +
> +
> +def test_generateLine_method():
> +    """
> +    Check inventory for method.
> +
> +    Methods are functions inside a class.
> +    """
> +    sut = make_SphinxInventory()
> +    parent = model.Class('ignore-system', 'class1', 'docstring')
> +
> +    result = sut._generateLine(
> +        model.Function('ignore-system', 'meth1', 'ignore-docstring', parent))
> +
> +    assert 'class1.meth1 py:method -1 class1.html#meth1 -\n' == result
> +
> +
> +def test_generateLine_attribute():
> +    """
> +    Check inventory for attributes.
> +    """
> +    sut = make_SphinxInventory()
> +    parent = model.Class('ignore-system', 'class1', 'docstring')
> +
> +    result = sut._generateLine(
> +        model.Attribute('ignore-system', 'attr1', 'ignore-docstring', parent))
> +
> +    assert 'class1.attr1 py:attribute -1 class1.html#attr1 -\n' == result
> +
> +
> +class UnknownType(model.Documentable):
> +    """
> +    Documentable type to help with testing.
> +    """
> +
> +
> +def test_generateLine_unknown():
> +    """
> +    When object type is uknown a message is logged and is handled as
> +    generic object.
> +    """
> +    log = []
> +    sut = make_SphinxInventory()
> +    sut.msg = lambda part, message: log.append((part, message))
> +
> +    result = sut._generateLine(
> +        UnknownType('ignore-system', 'unknown1', 'ignore-docstring'))
> +
> +    assert 'unknown1 py:obj -1 unknown1.html -\n' == result

Thanks so much for adding these.

Cheers,
mwh