Opened 4 years ago

Closed 4 years ago

#6892 enhancement closed fixed (fixed)

ISFTPServer should define avatar = Attribute()

Reported by: Adi Roiban Owned by: Julian Berman
Priority: normal Milestone:
Component: conch Keywords:
Cc: Adrian, z3p Branch: branches/isftpserver-avatar-attr-6892
branch-diff, diff-cov, branch-cov, buildbot
Author: julian

Description

The docstring clearly stated for ISFTPServer interface that

` The only attribute of this class is "avatar". `

but the interface definition lacks the

class ISFTPServer(Interface):
  avatar = Attribute('The avatar returned by the Realm that we are authenticated with, and represents the logged-in user.')

This makes it harder to test implementations of ISFTPServer using zope.interface validation methods.

Attachments (2)

6892-1.diff (1.9 KB) - added by Adi Roiban 4 years ago.
6892-2.diff (2.0 KB) - added by Adi Roiban 4 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 4 years ago by DefaultCC Plugin

Cc: z3p added

comment:2 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

I have added avatar as an Attribute, wrote a test for the new code, updated docstring and created a news file.

Hope I did not forget anything.

I will attach the patch.

Real time diff is here https://github.com/chevah/twisted/compare/6892-ISFTPServer-avatar.diff

Thanks!

Changed 4 years ago by Adi Roiban

Attachment: 6892-1.diff added

comment:3 Changed 4 years ago by Julian Berman

Keywords: review removed
Owner: set to Julian Berman

Hey!

I'd add the word "communication" to the docstring summary, and also, misc topfiles should be empty. Both of those are minor though, I'll just apply that change and merge, think this looks good.

Thanks!

comment:4 Changed 4 years ago by julian

Author: julian
Branch: branches/isftpserver-avatar-attr-6892

(In [41407]) Branching to isftpserver-avatar-attr-6892.

comment:5 Changed 4 years ago by Julian Berman

Owner: changed from Julian Berman to Adi Roiban

Sorry, spoke too soon.

I applied the patch, and modulo some other fixes (misplaced paren, and a twistedchecker error), it seems like the test that was added fails.

I'd suspect that probably what's meant is to check the client attribute on server, not server itself, but I'm going to volley this back just to be sure :).

Please resubmit with a fix, that one if it seems also to you to be the correct one after taking a look at the implementation there.

Changed 4 years ago by Adi Roiban

Attachment: 6892-2.diff added

comment:6 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: changed from Adi Roiban to Julian Berman

Sorry for the bad test. I have fixed it.

Not sure how I ended up with this. I think that somehow the error was shadowed by the known expectedFailures=2 for flatten tests.

I have attached a new diff.

Many thanks for the review.

comment:7 Changed 4 years ago by Julian Berman

Keywords: review removed

No worries, just wanted to make sure we were on the same page.

Thanks, looks good now!

comment:8 Changed 4 years ago by julian

Resolution: fixed
Status: newclosed

(In [41415]) Merge isftpserver-avatar-attr-6892: Add a missing attribute to the ISFTPServer interface.

Author: adiroiban Reviewers: Julian Fixes: #6892

Note: See TracTickets for help on using tickets.