Opened 11 months ago

Last modified 3 months ago

#6803 defect new

conch docstrings have incorrect return values

Reported by: Alex Owned by: lvh
Priority: normal Milestone:
Component: conch Keywords: documentation
Cc: z3p Branch: branches/conch-retval-docs-6803
(diff, github, buildbot, log)
Author: lvh Launchpad Bug:

Description

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

Attachments (2)

t6803.diff (2.9 KB) - added by Alex 11 months ago.
t6803.2.diff (3.0 KB) - added by Alex 11 months ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 11 months ago by DefaultCC Plugin

  • Cc z3p added

Changed 11 months ago by Alex

comment:2 Changed 11 months ago by thijs

  • Keywords documentation added; review removed
  • Owner set to Alex

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

Changed 11 months ago by Alex

comment:3 Changed 11 months ago by Alex

  • Keywords review added

comment:4 Changed 11 months ago by lvh

  • Owner Alex deleted

comment:5 Changed 11 months ago by lvh

  • Owner set to lvh

comment:6 Changed 11 months ago by lvh

  • Author set to lvh
  • Branch set to branches/conch-retval-docs-6803

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

comment:7 Changed 11 months 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: https://bugs.launchpad.net/pydoctor/+bug/1246713

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 11 months ago by lvh

  • Keywords review added
  • Owner lvh deleted

Here's the expected pydoctor failure:

http://buildbot.twistedmatrix.com/builders/documentation/builds/4106/steps/api-documentation/logs/new%20pydoctor%20errors

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 8 months ago by hynek

  • 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 https://www.dlitz.net/software/pycrypto/api/current/ , 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? http://twistedmatrix.com/trac/wiki/Plan/Python3 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 3 months 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.