Opened 3 years ago

Closed 3 years ago

#8240 enhancement closed fixed (fixed)

Improve type clarity of twisted.conch.ssh.userauth

Reported by: hawkowl Owned by: hawkowl
Priority: normal Milestone:
Component: conch Keywords:
Cc: z3p Branch: branches/clarity-sshuserauth-8240
branch-diff, diff-cov, branch-cov, buildbot
Author: hawkowl


As with #8237, make porting easier by improving clarity of types.

Change History (5)

comment:1 Changed 3 years ago by DefaultCC Plugin

Cc: z3p added

comment:2 Changed 3 years ago by hawkowl

Author: hawkowl
Branch: branches/clarity-sshuserauth-8240

(In [47035]) Branching to clarity-sshuserauth-8240.

comment:3 Changed 3 years ago by hawkowl

Keywords: review added

Okay, this one's a bit smaller.

Builders spun, please review.

comment:4 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

Thanks for the changes. Happy to see that twisted.conch got some attention

  1. interfaceToMethod changes. I think that this required a warning in case some is setting non bytes values.

For my project I am using custom checkers and they are defined as just str.

Maybe we can make this private and deprecate it, and add a helper for updating this structure. Maybe `SSHUserAuthServer.addCredentialsHandler(

Same for supportedAuthentications

I am talking about this differece in py2 vs py3

Python 3.4.3
>>> ca = {}
>>> ca['b'] = 'b'
>>> ca[b'b'] = b'b'
>>> ca
{'b': 'b', b'b': b'b'}

Python 2.7.6 
>>> ca = {}
>>> ca['b'] = 'b'
>>> ca[b'b'] = b'b'
>>> ca
{'b': 'b'}

I would say that changing from str to bytes without any deprecation warning is bad.

You can revert the type changes for interfaceToMethod and supportedAuthentications and merge

Otherwise please add your comments about why this is not breaking backward compatibility and resubmit it for review.

Many thanks!

comment:5 Changed 3 years ago by hawkowl

Resolution: fixed
Status: newclosed

(In [47044]) Merge clarity-sshuserauth-8240: Increase type clarity in twisted.conch.ssh.userauth

Author: hawkowl Reviewer: adiroiban Fixes: #8240

Note: See TracTickets for help on using tickets.