IResolver doesn't document its argument types or return values

[Glyph]

I was thinking of maybe implementing an ​IResolver, but the interface just says what its methods do in completely abstract terms, not how to get it done. (To its credit, it does say that things return a Deferred, but it doesn't say what the Deferred is supposed to fire with.)

From a combination of tracing a running twisted.names server and looking at the implementation of FileAuthority, it seems like the arguments are strings and ints, and the result is always a Deferred that fires with a 3-tuple of (results, authority, additional) lists, where each list is of twisted.names.dns.RRHeader objects. Is that right?

[Richard Wall]

Various twisted.names docstring improvements

[Richard Wall]

Ready for review in attachement:resolver-docs-4685.patch

• #4685 The most complete API documentation was in the various t.n.client.lookup* functions - so I moved that into the t.i.i.IResolver
• Added a few missing lookup methods to IResolver
• Moved the implements(IResolver) line to BaseResolver so that pydoctor can use the IResolver documentation for the various BaseResolver subclasses
• #2336 Implemented most of the changes suggested in comment 2 - making the various timeout argument defaults and documentation consistent between IResolver and its providers.
• Made a few whitespace changes to satisfy the coding standard.

[Jean-Paul Calderone]

This patch has tons of conflicts with trunk now. I started to resolve them, but then I decided I wasn't sure it was worthwhile, because the changes seem to be out of scope and perhaps just not the direction we want to go in anyway.

Can you break up this patch into several pieces - one piece which actually aims at resolving the problem described by this ticket, and other pieces for each of the other changes you think are improvements and attach those to new (or existing) tickets which describe the problem solved by each of those patches?

Generally, try to keep patches as small as possible and fix tickets individually rather than clumping together fixes for many tickets.

And as far as some of those other changes go, changes (or additions) to interfaces generally cannot be done like this. A major point of interfaces is to describe some API; if the interface changes from release to release, then this point is defeated.

Thanks again. Looking forward to your further work on this issue.

[Richard Wall]

An updated patch containing documentation mods for just two IResolver methods

[Richard Wall]

Replying to exarkun:

This patch has tons of conflicts with trunk now. I started to resolve them, but then I decided I wasn't sure it was worthwhile, because the changes seem to be out of scope and perhaps just not the direction we want to go in anyway.

Thanks exarkun - point taken.

See attachment:resolver-docs-4685-2.patch​

In that patch I've:

• Moved the documentation from t.n.client and t.n.common to IResolver.query and lookupAddress
• Rather than duplicated the documentation, I replaced the docstrings in client and common with an @see reference to the interface documentation.
• also fixed the spaces around the keyword args in the functions that I've touched.

So if I propose doing the same for the remaining IResolver methods and all the associated implementers.

I that be acceptable?

And as far as some of those other changes go, changes (or additions) to interfaces generally cannot be done like this. A major point of interfaces is to describe some API; if the interface changes from release to release, then this point is defeated.

I agree, I went a bit mad reformatting and changing unrelated code.

One thing I noticed is that both implementations of IResolverSimple.getHostByName add an "effort" argument...so I assumed that someone had forgotten to add it to the interface method.

Shall I open a ticket for that?

Let me know what you think.

-RichardW.

[Richard Wall]

Sorry the other issue is mentioned in #2336.

That IResolver methods each have a @type timeout: C{int} and default value 10

But the implementations all have @type param: Sequence of C{int} with default value None.

This is why my original patch also tried to address the points in #2336

So is IResolver correct - in which case shall I document the timeout params exactly as they are in IResolver - and in which case the implementations are deviating from the interface.

Or should the IResolver timeout parameters be "fixed" to match the implementations in twisted - so changing the published API?

[Jean-Paul Calderone]

In that patch I've: Moved the documentation from t.n.client and t.n.common to IResolver.query and lookupAddress Rather than duplicated the documentation, I replaced the docstrings in client and common with an @see reference to the interface documentation. also fixed the spaces around the keyword args in the functions that I've touched.

I like these changes.

That IResolver methods each have a @type timeout: C{int} and default value 10 But the implementations all have @type param: Sequence of C{int} with default value None. This is why my original patch also tried to address the points in #2336 So is IResolver correct - in which case shall I document the timeout params exactly as they are in IResolver - and in which case the implementations are deviating from the interface.

Meh. A bit of a mess. The interface should be fixed to agree with the implementation, not the other way around. (This effectively makes it a documentation fix. If we did it the other way, we would break all code trying to use timeouts with these implementations.)

One thing I noticed is that both implementations of IResolverSimple.getHostByName add an "effort" argument...so I assumed that someone had forgotten to add it to the interface method.

I don't think it's worth adding this to the interface. The extra parameter supported by the implementation doesn't get in anyone's way. And getHostByName usage should be superseded by some kind of getaddrinfo usage. We should just try to do that interface right. Perhaps a note about that is worth adding to the getaddrinfo ticket - #4362.

Thanks!

[Richard Wall]

All the lookup methods given the same treatment as in previous patch.

[Richard Wall]

Found some more implementations and documentation for lookupZone

[Richard Wall]

Ready for review in attachment:resolver-docs-4685-4.patch​

1. Moved the documentation from t.n.client to t.i.interfaces.IResolver
2. Added two missing methods to IResolver - lookupSenderPolicy and lookupNamingAuthorityPointer
3. Moved some adhoc comments about lookupZone into the IResolver docstring.
4. Replaced existing docstrings in t.n.client and t.n.common.BaseResolver with an @see references to the IResolver documentation.
5. Fixed the spaces around the timeout keyword args in the functions that I touched.
6. Changed IResolver.lookup* timeout defaults to None - to match the t.n.client.lookup* functions.
7. ...including the IResolver.lookupZone timeout, whose default was already 10 but is now None but documented as type Int.

NB:

• I think this patch also resolves most of the points in #2336.
• I created an empty news file, but it doesn't seem to get included in the patch.

[Tom Prince]

(In [37133]) Branching to IResolver-docs-4685.

[Tom Prince]

<tomprince> exarkun: re: #4685 Would the fact that all the providers and consumers in twisted of
    an interface expect things not in the interface be grounds for considering the lack a bug,
    rather than adding them being an incompatability? (not saying that that is the case here, just
    something that I was considering looking into here)
<exarkun> tomprince: Yes, that's at least a very strong argument for changing an existing
    interface.

[Tom Prince]

1. @see annotations should use L{} to generate links.
2. It appears that most of the classes implementing IResolver don't document that fact (so that the apidocs don't inherit the docuemntation. It may be that ResolverBase simply needs to be made to implement the interface. (Please file a ticket for fixing this, as well as tests to make sure they actually satisfy the interface)
   • It occurs to me that if pydoctor were to support moduleProvides, then marking twisted.names.client as providing IResolver would solve to give documentation to the free functions there. (except that t.n.client doesn't implement query).

Thanks for your work on this. Please re-submit for review (with patch against the branch), after addressing the above issues.

3. One other thing, there is a lot of duplication in the docstrings of IResolverBase. It would be nice if that could be factored, so that all the methods don't need to be changed, if something changes. However, it probably isn't worth the complexity it would entail to implement and test this.

[Richard Wall]

Replying to tom.prince:

1. @see annotations should use L{} to generate links.

Done.

1. It appears that most of the classes implementing IResolver don't document that fact (so that the apidocs don't inherit the docuemntation. It may be that ResolverBase simply needs to be made to implement the interface. (Please file a ticket for fixing this, as well as tests to make sure they actually satisfy the interface)

I've done it here and added a test.

• It occurs to me that if pydoctor were to support
• moduleProvides, then marking twisted.names.client as
• providing IResolver would solve to give documentation to the
• free functions there. (except that t.n.client doesn't
• implement query).

Added moduleProvides to client and a corresponding test.

Thanks for your work on this. Please re-submit for review (with patch against the branch), after addressing the above issues.

3. One other thing, there is a lot of duplication in the docstrings of IResolverBase. It would be nice if that could be factored, so that all the methods don't need to be changed, if something changes. However, it probably isn't worth the complexity it would entail to implement and test this.

I'm not sure how I'd do that. Are you thinking that the IResolver class could be generated dynamically? Would that work with pydoctor?

If you have an idea how to do it sounds like something worth adding to a new ticket.

I'm attaching a patch for now, I'll update the branch and start a build when I have a better internet connection.

See attachment:resolver-docs-4685-5.patch​

[Richard Wall]

(In [37197]) apply resolver-docs-4685-5.patch. refs #4685

[Richard Wall]

Ok ready for another review in source:branches/IResolver-docs-4685

• See comment:13, where I addressed the points in the last review
• Applied those to the branch
• Fixed a couple of other pydoctor and twisted-checker issues.
• ​Forced build which looks ok except that twisted-checker now complains about missing @param documentation in the methods which now simply refer to the interface docs. I guess that's an area where twisted-checker can be improved.

[Tom Prince]

Re-reading my previous review, I realize I wasn't entirely clear, sorry.

If a method isn't documented, but is documented in an interface that the class documents, pydoctor is smart enough to inherit the documentation. Thus, since things are now marked with @implementer, having documentation with just @see leads to worse documentation. (Incidentally, that should also fix some of the twistedchecker errors, I think.)

That fact prompted my speculation about moduleProvides was that if pydoctor supported it, that would lead to better documentation of the free functions. (Unfortunately that isn't the case). I think that since it doesn't, adding here is beyond the scope of this ticket (particularly since it would be the only use of it in twisted).

Please commit, after addressing the following points.

1. Please remove the @see annotations on ResolverBase and subclasses.
   • And, check in the generated documentation (buildbot will do this for you) that the IResolver methods are properly documented on ResolverBase and subclasses.
2. Remove the moduleProvides and free-function query. (But feel free to open a ticket for doing that, and ticket with pydoctor to support it)
3. Add a test that ResolverBase implements IResolver.

[Richard Wall]

(In [37241]) Branching to 'IResolver-docs-4685-2'

[Richard Wall]

Replying to tom.prince: <snip>

Please commit, after addressing the following points.

1. Please remove the @see annotations on ResolverBase and subclasses. * And, check in the generated documentation (buildbot will do this for you) that the IResolver methods are properly documented on ResolverBase and subclasses.

Done.

1. Remove the moduleProvides and free-function query. (But feel free to open a ticket for doing that, and ticket with pydoctor to support it)

Done.

See

• ​https://bugs.launchpad.net/pydoctor/+bug/1132527
• #6328

1. Add a test that ResolverBase implements IResolver.

There was already one there, but I moved the client.Resolver interface test to test_client.py instead of test_names.py.

I also added coding standard whitespace between all the classes and functions that I've visited in this branch.

Also note that the ResolverBase.getHostByName api docs go missing in this branch because of

• #6329
• ​https://bugs.launchpad.net/pydoctor/+bug/1132535

But the original documentation contained a missing link anyway.

* ​https://twistedmatrix.com/documents/12.3.0/api/twisted.names.common.ResolverBase.html#getHostByName

Note that this branch results in a regression in the twisted-checker build tests but that's because twisted-checker doesn't recognise when api documentation will be inherited from a zope interface. I've reported it:

• ​https://bugs.launchpad.net/twistedchecker/+bug/1132540

Is it ok to merge to trunk despite the problems above?

See:

source:branches/IResolver-docs-4685-2

[Richard Wall]

​Build results here

[Tom Prince]

The temporary regression is fine, merge away.

[Richard Wall]

(In [37286]) Merge IResolver-docs-4685-2: Updated IResolver docstrings

Author: rwall Reviewer: exarkun, tom.prince Fixes: #4685

The API documentation for IResolver and its implementations has been updated and consolidated in twisted.internet.interfaces.IResolver.