Opened 12 years ago
Closed 9 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)
Change History (26)
comment:1 Changed 11 years ago by
Owner: | Jean-Paul Calderone deleted |
---|
comment:2 Changed 10 years ago by
Keywords: | documentation added |
---|
Changed 10 years ago by
Attachment: | resolver-docs-4685.patch added |
---|
comment:3 Changed 10 years ago by
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 follow-up: 6 Changed 9 years ago by
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 9 years ago by
Status: | new → assigned |
---|
Changed 9 years ago by
Attachment: | resolver-docs-4685-2.patch added |
---|
An updated patch containing documentation mods for just two IResolver methods
comment:6 Changed 9 years ago by
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 9 years ago by
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 9 years ago by
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 9 years ago by
Attachment: | resolver-docs-4685-3.patch added |
---|
All the lookup methods given the same treatment as in previous patch.
Changed 9 years ago by
Attachment: | resolver-docs-4685-4.patch added |
---|
Found some more implementations and documentation for lookupZone
comment:9 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Richard Wall deleted |
Status: | assigned → new |
Ready for review in attachment:resolver-docs-4685-4.patch
- Moved the documentation from t.n.client to t.i.interfaces.IResolver
- Added two missing methods to IResolver - lookupSenderPolicy and lookupNamingAuthorityPointer
- Moved some adhoc comments about lookupZone into the IResolver docstring.
- Replaced existing docstrings in t.n.client and t.n.common.BaseResolver with an @see references to the IResolver documentation.
- Fixed the spaces around the timeout keyword args in the functions that I touched.
- Changed IResolver.lookup* timeout defaults to None - to match the t.n.client.lookup* functions.
- ...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 9 years ago by
Author: | rwall → tomprince, rwall |
---|---|
Branch: | → branches/IResolver-docs-4685 |
(In [37133]) Branching to IResolver-docs-4685.
comment:11 Changed 9 years ago by
<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 follow-up: 13 Changed 9 years ago by
Author: | tomprince, rwall → rwall |
---|---|
Keywords: | review removed |
Owner: | set to Richard Wall |
@see
annotations should useL{}
to generate links.- 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 thatResolverBase
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 markingtwisted.names.client
as providingIResolver
would solve to give documentation to the free functions there. (except thatt.n.client
doesn't implementquery
).
- It occurs to me that if pydoctor were to support
Thanks for your work on this. Please re-submit for review (with patch against the branch), after addressing the above issues.
- 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 Changed 9 years ago by
Replying to tom.prince:
@see
annotations should useL{}
to generate links.
Done.
- 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 thatResolverBase
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 markingtwisted.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.
- 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.
Changed 9 years ago by
Attachment: | resolver-docs-4685-5.patch added |
---|
comment:15 Changed 9 years ago by
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 follow-up: 18 Changed 9 years ago by
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.
- Please remove the
@see
annotations onResolverBase
and subclasses.- And, check in the generated documentation (buildbot will do this for you) that the
IResolver
methods are properly documented onResolverBase
and subclasses.
- And, check in the generated documentation (buildbot will do this for you) that the
- Remove the
moduleProvides
and free-functionquery
. (But feel free to open a ticket for doing that, and ticket with pydoctor to support it) - Add a test that
ResolverBase
implementsIResolver
.
comment:17 Changed 9 years ago by
Branch: | branches/IResolver-docs-4685 → branches/IResolver-docs-4685-2 |
---|
(In [37241]) Branching to 'IResolver-docs-4685-2'
comment:18 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Richard Wall deleted |
Replying to tom.prince: <snip>
Please commit, after addressing the following points.
- Please remove the
@see
annotations onResolverBase
and subclasses. * And, check in the generated documentation (buildbot will do this for you) that theIResolver
methods are properly documented onResolverBase
and subclasses.
Done.
- Remove the
moduleProvides
and free-functionquery
. (But feel free to open a ticket for doing that, and ticket with pydoctor to support it)
Done.
See
- Add a test that
ResolverBase
implementsIResolver
.
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:20 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Richard Wall |
The temporary regression is fine, merge away.
comment:21 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
Various twisted.names docstring improvements