Merge lp:~adiroiban/pydoctor/1287458-objects.inv into lp:~mwhudson/pydoctor/dev

[Adi Roiban]

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.

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.

I have added some tests and updated documentation.

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!

[Michael Hudson-Doyle (mwhudson)]

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.m...

[lp:~adiroiban/pydoctor/1287458-objects.inv]

616. By Adi Roiban <email address hidden> on 2014-05-12

Update after review.

[Adi Roiban (adiroiban)]

>> 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...

This is the default... not sure why Sphinx configuration allows a custom file name.. maybe for extra flexibility.
I think that we can start with simple default and this can be later extended.

>> 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 :-)

OK. I have refactored this to use system.projectname.

I am not sure why we still have projectname and guessedprojectname.

Please check that I have not done anything stupid.

>> + 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?
>
Done!

Thanks for the review!

[Michael Hudson-Doyle (mwhudson)]

Adi Roiban <email address hidden> writes:

>>> 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...
>
> This is the default... not sure why Sphinx configuration allows a custom file name.. maybe for extra flexibility.
> I think that we can start with simple default and this can be later extended.

+1

>
>>> 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 :-)
>
> OK. I have refactored this to use system.projectname.
>
> I am not sure why we still have projectname and guessedprojectname.

Thanks.

> Please check that I have not done anything stupid.

It looks OK.

>>> + 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?
>>
> Done!

Argh, so this was probably a bad suggestion, because .kind is actually
quite varied -- it can get set to, for example, "Instance Variable" or
"Class Method". So what had before was probably better.

But, to rescue myself a bit, you only need to do this to determine the
"domain". I think you can construct the link part by examining
obj.documentation_location -- see taglink in templatewriter/utils.py
(which could/should probably have the link computation part extracted --
I don't know why the 'link' function in there is like that, it's
misleadingly named :(). This also makes me realize that objects.inv
generation should take obj.isVisible into account...

Sorry for not mentioning this stuff earlier -- do you want to make these
changes? I can probably do it too, now the brain cells are warmed up a
bit.

Cheers,
mwh

[Adi Roiban (adiroiban)]

Regarding obj,kind obj.documentation_location, if you have time, please do the refactor.

If you don't have time, I can also do this... but I will be busy until 1 of June.

=== modified file 'README.txt'
--- README.txt	2013-05-29 03:28:59 +0000
+++ README.txt	2014-05-12 01:26:49 +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-12 01:26:49 +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,9 @@
                 'warning',
                 'WARNING: guessing '+name+' for project name', thresh=-1)
             system.guessedprojectname = name
+            system.projectname = name
+        else:
+            system.projectname = system.options.projectname
 
         # step 4: save the system, if desired
 
@@ -391,6 +395,17 @@
                 f.close()
                 system.options = options
 
+            # Generate Sphinx inventory.
+            sphinx_inventory = SphinxInventory(
+                logger=system.msg,
+                project_name=system.projectname,
+                )
+            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-12 01:26:49 +0000
@@ -0,0 +1,98 @@
+"""
+Support for Sphinx compatibility.
+"""
+from __future__ import absolute_import
+import os
+import zlib
+
+
+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()
+        full_parent_name = ''
+        if obj.parent:
+            full_parent_name = obj.parent.fullName()
+        display = '-'
+        mapping = {
+            'package': ('module', full_name, None),
+            'module': ('module', full_name, None),
+            'class': ('class', full_name, None),
+            'method': ('method', full_parent_name, obj.name),
+            'function': ('function', full_parent_name, obj.name),
+            'attribute': ('attribute', full_parent_name, obj.name),
+            }
+
+        try:
+            domainname, base_url, anchor = mapping[obj.kind.lower()]
+        except (KeyError, AttributeError):
+            domainname = 'obj'
+            base_url = full_name
+            anchor = None
+            self.msg('sphinx', "Unknown type for %s." % (full_name,))
+
+        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-12 01:26:49 +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