Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#7302 defect closed fixed (fixed)

IPolicyForHTTPS.createForNetloc incorrectly documents its hostname parameter's type

Reported by: habnabit Owned by: habnabit
Priority: normal Milestone:
Component: web Keywords:
Cc: jknight Branch: branches/fix-IPolicyForHTTPS-documentation-7302
branch-diff, diff-cov, branch-cov, buildbot
Author: habnabit

Description

IPolicyForHTTPS.createForNetloc states that the hostname parameter should be of type unicode. However, both of its implementors seem to disagree. BrowserLikePolicyForHTTPS documents that its hostname parameters is bytes and attempts hostname.decode('ascii'). _DeprecatedToCurrentPolicyForHTTPS similarly documents its hostname parameter is bytes. The current primary user of the interface, Agent, does not explicitly document that it passes bytes, but regardless only ever passes bytes to createForNetloc. One test does document the parameter as unicode, but then passes bytes anyway.

Based on the above, it's my opinion that the action to be taken is to change the documentation to be consistent with the implementation.

Change History (14)

comment:1 Changed 8 years ago by DefaultCC Plugin

Cc: jknight added

comment:2 Changed 8 years ago by habnabit

Author: habnabit
Branch: branches/fix-IPolicyForHTTPS-documentation-7302

(In [42612]) Branching to 'fix-IPolicyForHTTPS-documentation-7302'

comment:3 Changed 8 years ago by habnabit

(In [42613]) Update 'unicode' -> 'bytes'.

refs #7302

comment:4 Changed 8 years ago by habnabit

Keywords: review added

comment:5 Changed 8 years ago by Jeremy Thurgood

Related to this, BrowserLikePolicyForHTTPS.createForNetloc documents a tls parameter which isn't present.

comment:6 Changed 8 years ago by Jean-Paul Calderone

Type: enhancementdefect

comment:7 Changed 8 years ago by Jean-Paul Calderone

Related to this, BrowserLikePolicyForHTTPS.createForNetloc documents a tls parameter which isn't present.

Please file a ticket for it instead of commenting on an already-up-for-review ticket which a different scope. Thanks!

comment:8 Changed 8 years ago by Jean-Paul Calderone

Milestone: Twisted 14.1.0

comment:9 Changed 8 years ago by Adi Roiban

This looks good. Thanks for the fix.

Since BrowserLikePolicyForHTTPS implements IPolicyForHTTPS, does it need to duplicate docstring for methods which are already documented in the interface? Maybe a See iweb.BrowserLikePolicyForHTTPS.createForNetloc can help with reducing code duplication.

Thanks!

comment:10 in reply to:  9 Changed 8 years ago by Glyph

Replying to adiroiban:

This looks good. Thanks for the fix.

Since BrowserLikePolicyForHTTPS implements IPolicyForHTTPS, does it need to duplicate docstring for methods which are already documented in the interface? Maybe a See iweb.BrowserLikePolicyForHTTPS.createForNetloc can help with reducing code duplication.

Nope. pydoctor will copy the interface docstring if it implements the interface, so it's actually better to leave it blank.

comment:11 Changed 8 years ago by Glyph

Keywords: review removed
Owner: set to habnabit

Sigh. URLs are weird. I think maybe this was a mistake? But you're right in that it's a mistake that was already made, and we should fix the documentation to reflect reality. Fixing the mistake for real might involve IRIs or something.

Make it so.

comment:12 Changed 8 years ago by Glyph

Ahem. Ping. Please land within 24 hours or I will.

comment:13 Changed 8 years ago by Glyph

Resolution: fixed
Status: newclosed

(In [42954]) Merge fix-IPolicyForHTTPS-documentation-7302: fix interface type documentation

Author: habnabit

Reviewer: glyph

Fixes: #7302

IPolicyForHTTPS.createForNetloc states that the hostname parameter should be of type unicode. However, both of its implementors seem to disagree. This change adjusts the interface documentation to reflect the reality of the implementation.

comment:14 Changed 7 years ago by hawkowl

Milestone: Twisted 14.1.0

Ticket retargeted after milestone deleted

Note: See TracTickets for help on using tickets.