Opened 5 years ago

Last modified 4 years ago

#6803 defect new

conch docstrings have incorrect return values

Reported by: Alex Gaynor Owned by: lvh
Priority: normal Milestone:
Component: conch Keywords: documentation
Cc: z3p Branch: branches/conch-retval-docs-6803
branch-diff, diff-cov, branch-cov, buildbot
Author: lvh


This covers a series of errors in twisted.conch.ssh.keys

Attachments (2)

t6803.diff (2.9 KB) - added by Alex Gaynor 5 years ago.
t6803.2.diff (3.0 KB) - added by Alex Gaynor 5 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 5 years ago by DefaultCC Plugin

Cc: z3p added

Changed 5 years ago by Alex Gaynor

Attachment: t6803.diff added

comment:2 Changed 5 years ago by Thijs Triemstra

Keywords: documentation added; review removed
Owner: set to Alex Gaynor

Thanks for your patch. All of the changes are wrapped in C{} but should be using L{} instead.

Changed 5 years ago by Alex Gaynor

Attachment: t6803.2.diff added

comment:3 Changed 5 years ago by Alex Gaynor

Keywords: review added

comment:4 Changed 5 years ago by lvh

Owner: Alex Gaynor deleted

comment:5 Changed 5 years ago by lvh

Owner: set to lvh

comment:6 Changed 5 years ago by lvh

Author: lvh
Branch: branches/conch-retval-docs-6803

(In [40529]) Branching to 'conch-retval-docs-6803'

comment:7 Changed 5 years ago by lvh

Keywords: review removed

Thank you for your patch! Everything in there looks good, but while reading I decided to clean that module up a bit more extensively, since it was in dire need of some love (it still is, but those are more in-depth changes).

While cleaning up, I have found some stuff for which I have filed separate tickets:

  • conch.ssh.keys.fromString raises BadKeyError instead of EncryptedKeyError: #6804
  • t.conch.ssh.keys.Key._guessStringType is a classmethod, should be a function: #6805
  • t.conch.ssh.keys.Key._guessStringType should return upper case: #6806

There's also an issue with the API docs that pydoctor can't resolve L{Crypto.whatever}. That's a bug in pydoctor, though, so I reported it there:

Remaining issues: __init__ insists that it can only take a pubkey object. That doesn't seem right, plenty of the classmethods feed it private key objects, too. There's a few other cases where the class seems confused about itself.

There's a part (which I marked with XXX) where the code does if not passphrase:; I think it means if passphrase is None, since that's the default value. I don't think it matters, because an empty passphrase is how you specify "don't encrypt" in most UIs I know of, but I still think that's what the code intends to say.

comment:8 Changed 5 years ago by lvh

Keywords: review added
Owner: lvh deleted

Here's the expected pydoctor failure:

However, this persists:

twisted.conch.ssh.keys.Key.toString: invalid ref to NoneType

Which I really don't understand, because there are plenty of identical references to NoneType it doesn't complain about, and it really looks like L{NoneType<types.NoneType>} ought to work...

That said, I think this is ready for reviewing, with all the above caveats :)

comment:9 Changed 4 years ago by Hynek Schlawack

Keywords: review removed
  • your NoneType problem appears resolved now?
  • Crypto.PublicKey.pubkey.pubkey this is a pretty mysterious type that appears to be valid but pydoctor can’t resolve it (and I really would love to know how it fits into , but that’s a story for another day).
  • 570: Shouldn’t it be “The user *re*presentation of this L{Key}'s fingerprint.” ?
  • 512: “of the public key”, no?
  • you did replace %-formatting with .format() inconsistently, is there any system behind it?
  • it also seems to me, that that’s rather out of scope for this ticket. :)
  • 937: messageLength should be an L{int}.
  • 952: longs have been abandoned as of Python 3. Do we really want to keep them around in docs? doesn’t say anything unfortunately.
  • 103: I’m pretty sure to whatever you’re referring to, it’s not a “he”. :)

I’m afraid I’ve opened a few question, I’d especially appreciate if we could clarify Crypto.PublicKey.pubkey.pubkey a bit, I’ve grepped Twisted for it and didn’t find anything helpful. As for the format fixes, I don’t feel very strongly about them but I’d guess that others may.

comment:10 Changed 4 years ago by Glyph

Owner: set to lvh

A bit of meta-review:

hynek, it looks like you reviewed this without reassigning it to an author, so it got lost.

lvh, you probably shouldn't have expanded the scope of the ticket so much, just landing the few cleanups from the beginning would have been an immediate improvement. But since you decided to make this bed maybe you should have a nap in it ;-). Assigning to you to fix it up...

Note: See TracTickets for help on using tickets.