Opened 7 years ago

Closed 5 years ago

#4685 enhancement closed fixed (fixed)

IResolver doesn't document its argument types or return values

Reported by: Glyph Owned by: Richard Wall
Priority: normal Milestone:
Component: names Keywords: documentation
Cc: Richard Wall Branch: branches/IResolver-docs-4685-2
branch-diff, diff-cov, branch-cov, buildbot
Author: rwall

Description

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?

Attachments (5)

resolver-docs-4685.patch (52.0 KB) - added by Richard Wall 5 years ago.
Various twisted.names docstring improvements
resolver-docs-4685-2.patch (4.5 KB) - added by Richard Wall 5 years ago.
An updated patch containing documentation mods for just two IResolver methods
resolver-docs-4685-3.patch (43.4 KB) - added by Richard Wall 5 years ago.
All the lookup methods given the same treatment as in previous patch.
resolver-docs-4685-4.patch (44.4 KB) - added by Richard Wall 5 years ago.
Found some more implementations and documentation for lookupZone
resolver-docs-4685-5.patch (17.3 KB) - added by Richard Wall 5 years ago.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 7 years ago by <automation>

Owner: Jean-Paul Calderone deleted

comment:2 Changed 5 years ago by Thijs Triemstra

Keywords: documentation added

Changed 5 years ago by Richard Wall

Attachment: resolver-docs-4685.patch added

Various twisted.names docstring improvements

comment:3 Changed 5 years ago by Richard Wall

Author: rwall
Cc: Richard Wall added
Keywords: review added

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.

comment:4 Changed 5 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Richard Wall

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.

comment:5 Changed 5 years ago by Richard Wall

Status: newassigned

Changed 5 years ago by Richard Wall

Attachment: resolver-docs-4685-2.patch added

An updated patch containing documentation mods for just two IResolver methods

comment:6 in reply to:  4 Changed 5 years ago by 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.

comment:7 Changed 5 years ago by 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?

comment:8 Changed 5 years ago by 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!

Changed 5 years ago by Richard Wall

Attachment: resolver-docs-4685-3.patch added

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

Changed 5 years ago by Richard Wall

Attachment: resolver-docs-4685-4.patch added

Found some more implementations and documentation for lookupZone

comment:9 Changed 5 years ago by Richard Wall

Keywords: review added
Owner: Richard Wall deleted
Status: assignednew

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.

comment:10 Changed 5 years ago by Tom Prince

Author: rwalltomprince, rwall
Branch: branches/IResolver-docs-4685

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

comment:11 Changed 5 years ago by 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.

comment:12 Changed 5 years ago by Tom Prince

Author: tomprince, rwallrwall
Keywords: review removed
Owner: set to Richard Wall
  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.

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

comment:13 in reply to:  12 Changed 5 years ago by 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.

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

Changed 5 years ago by Richard Wall

Attachment: resolver-docs-4685-5.patch added

comment:14 Changed 5 years ago by Richard Wall

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

comment:15 Changed 5 years ago by Richard Wall

Keywords: review added
Owner: Richard Wall deleted

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.

comment:16 Changed 5 years ago by Tom Prince

Keywords: review removed
Owner: set to Richard Wall

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.

comment:17 Changed 5 years ago by Richard Wall

Branch: branches/IResolver-docs-4685branches/IResolver-docs-4685-2

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

comment:18 in reply to:  16 Changed 5 years ago by Richard Wall

Keywords: review added
Owner: Richard Wall deleted

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

  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

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:

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

See:

source:branches/IResolver-docs-4685-2

comment:19 Changed 5 years ago by Richard Wall

comment:20 Changed 5 years ago by Tom Prince

Keywords: review removed
Owner: set to Richard Wall

The temporary regression is fine, merge away.

comment:21 Changed 5 years ago by Richard Wall

Resolution: fixed
Status: newclosed

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

Note: See TracTickets for help on using tickets.