Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed implementation of interface role/object type #11

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

a3kov
Copy link

@a3kov a3kov commented Jun 29, 2017

Fixes #9 and #10.

Analysis of the current problems:

  • missing interface role, which breaks intersphinx cross references and probably other things
  • 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.

Summary of changes:

  • Most importantly, now you can reference interface docs directly using :interface: form, in addition to :class:
  • With the patch fuzzy search works both for :class: and :interface: roles
  • Intersphinx references for interfaces now work correctly both when using :class: and :interface: reference

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 sphinx-doc/sphinx#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
Copy link
Contributor

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
Copy link
Author

a3kov commented Jun 30, 2017

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
Copy link
Contributor

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
Copy link
Author

a3kov commented Jun 30, 2017

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:

  • they can stop using this package
  • they can implement another package which will do renames (if it's possible at all) , leaving interface for other things. The only requirement is they can't inter-operate (e.g. using intersphinx) with other docs, which use this package, because of the name conflict - interface in this case will mean different things depending on target

@jamadden
Copy link
Contributor

jamadden commented Jun 30, 2017

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
Copy link
Author

a3kov commented Jun 30, 2017

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
Copy link
Author

a3kov commented Jun 30, 2017

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
Copy link
Author

a3kov commented Jun 30, 2017

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:

  • it may require a lot more code
  • the scope of this package is only documenting interfaces, not all Zope-ish things
  • it's not obvious for me how cross-domain object search would work - we need to make sure not to break current docs - :class: references should reference zope:interface objects. It may be not easy at all.
  • even if we move interface role to zope domain, we can't rename/move interface directive, because it will break existing docs. Then we will have an issue that the role and the directive are in different domains, which looks like a huge can of worms to me.

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
Copy link
Contributor

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
Copy link
Author

a3kov commented Jun 30, 2017

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

@a3kov
Copy link
Author

a3kov commented Jun 30, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants