repoze / repoze.sphinx.autointerface

Auto-generate Sphinx API docs from Zope interfaces
http://pypi.python.org/pypi/repoze.sphinx.autointerface
Other
8 stars 7 forks source link

Fixed implementation of interface role/object type #11

Open a3kov opened 7 years ago

a3kov commented 7 years ago

Fixes #9 and #10.

Analysis of the current problems:

Summary of changes:

Note: fuzzy search works fully only for local references. It will not work with intersphinx references, except when used in context (you can read about the latter feature implemented in Sphinx 1.6.2 here https://github.com/sphinx-doc/sphinx/pull/3425)

I've also cleaned up the code a bit, and added a comment explaining the code for the next developer working with it.

jamadden commented 7 years ago

class object type is incorrectly patched. The current code makes interface object type reference-able by the class role, which is correct. But it also makes class object type reference-able by the interface role, so you can reference a class which is not a proper interface - incorrect behavior.

I disagree. It's certainly possible for classes to be thought of as interfaces. Many of the collections ABCs like Container are good examples. And since they typically have __subclasshook__ to simply check to see if a method is present, inheritance doesn't even have to be involved. So a docstring like "Accepts a :interface:collections.Container" is perfectly reasonable.

a3kov commented 7 years ago

We are not talking about interface as a programming concept. In the context of this package, interface is a Zope Interfaces interface. It has to be declared as one, so arbitrary classes are not always interfaces. Maybe I'm wrong - I'm not very familiar with Zope, but this is what I get from reading https://docs.zope.org/zope.interface/README.html#defining-interfaces That's why this package implements specific directive to document interface, and it makes sense to only refer objects created by the directive as interfaces

jamadden commented 7 years ago

We are not talking about interface as a programming concept.

I certainly don't see why not. If this package is going to take over the very generic role name interface, it ought to allow for generic usage of it.

a3kov commented 7 years ago

Well, namespace conflicts is not something I wanted to deal with. The current naming convention was picked long time ago and we are not in position to change it - it's too late. However, if someone is concerned that generic name refers to a Zope concept, they have 2 options:

jamadden commented 7 years ago

Let's try looking at it this way: does being able to use :interface: to refer to a non-Interface object cause any actual problems? Or is it just an argument from purity? I don't see any actual problems that it causes, and "practicality beats purity."

To me, it's very practical that I don't have to care about what the thing that I'm referencing "actually" is. If it makes the documentation clearer to say ":interface:", because that's the intent of the code, I'd like to be able to say that; forcing a distinction seems arbitrary, without any benefits that I can see; indeed, one could even argue that it in throwing away duck-typing it'sun-Pythonic. It's similar to being able to use both :exc: and :class: to refer to TypeError. Sometimes the documentation makes more sense with one, sometimes the other.

EDIT: Full disclosure, I don't think I've ever, or at least not in a long time, used :interface: to reference anything. I always use :class:. So this is a somewhat theoretical argument for me too!

a3kov commented 7 years ago

For me it's both practical and correct that :interface: refers to a :interface: documentation, not a :class: documentation, and if someone by mistake refers to a generic non-interface class, it will not work. For me it's explicit and obvious behavior, naturally expected, because interface is a specific version of a class. More specific objects should be reference-able by their generic role variants, but not the other way. Every documented interface (inside this package) is a class, but not every class is an interface, and this is reflected in this patch.

Full disclosure, I don't think I've ever, or at least not in a long time, used :interface: to reference anything. I always use :class:. So this is a somewhat theoretical argument for me too!

The patch implements support for this feature. Before :interface: role was missing, and references like this did not work at all.

a3kov commented 7 years ago

Note, if someone doesn't care what is the object he referring to, he may use :any: role. Or in the Python domain, :obj:. In this case, the user's intent is clear: give me a link to something I don't even care what it is.

a3kov commented 7 years ago

I think for Zope-ish concepts the more correct approach in the long term would probably be to implement a separate domain. Then you would refer specific namespaced terms e.g. zope:interface. However:

So I will wait for the maintainer's verdict on this. But I guess keeping things simple and using generic name in the python domain is the best approach.

jamadden commented 7 years ago

Then we're back to the central argument: :interface: means much more than just a Zope interface. I don't see why this package should start restricting that artificially.

I'm arguing this partly because it was apparently my patch that made sure that worked in the first place, way back in 2012. Presumably there was a reason, something that I or my co-workers were doing at the time that made that necessary.

This all turns out to be somewhat moot. I checked both Sphinx 1.5 and Sphinx 1.6 with the released version of repoze.sphinx.autointerface, and any :interface: reference is rendered badly, whether to a class, an Interface and using a partial or complete name: ref1

This patch at least makes the Interface case render correctly. Yay!

However there is an issue: This patch breaks with Sphinx 1.5 (which is still widely deployed as the default on readthedocs):

sphinx-build -b html -d _build/doctrees   . _build/html
Running Sphinx v1.5.6

Exception occurred:
  File "//repoze.sphinx.autointerface/repoze/sphinx/autointerface.py", line 107, in setup
    current_domain = app.registry.domains['py']
AttributeError: 'Sphinx' object has no attribute 'registry'
a3kov commented 7 years ago

@jamadden Thanks for the test! You are right, I did not think about old versions. Will add some error checking

a3kov commented 7 years ago

Then we're back to the central argument: :interface: means much more than just a Zope interface. I don't see why this package should start restricting that artificially.

Well we already have generic interface directive, and it won't go away. So I suspect someone arguing about generic names should travel back in time and warn the developers about future name conflicts. :P