Opened 11 years ago

Closed 7 years ago

Last modified 7 years ago

#3179 defect closed fixed (fixed)

DccChatFactory fails to return protocol

Reported by: ldlework Owned by:
Priority: normal Milestone:
Component: words Keywords: words irc dcc chat easy
Cc: Branch: branches/dccchatfactory-protocol-3179
branch-diff, diff-cov, branch-cov, buildbot
Author: cyli, ldlework

Description

Line 1718 of /twisted/words/protocols/irc.py should be :

        return p

Attachments (4)

irc.py.diff (480 bytes) - added by Craig Nagy 7 years ago.
test_irc.py.diff (979 bytes) - added by Craig Nagy 7 years ago.
test_irc.py.2.diff (880 bytes) - added by Craig Nagy 7 years ago.
Whitespace fixed. Ignore previous diff.
3179.diff (1.6 KB) - added by Craig Nagy 7 years ago.
Diff for all changes, now includes a news file.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 8 years ago by <automation>

Owner: Jean-Paul Calderone deleted

comment:2 Changed 7 years ago by moijes12

Keywords: easy added

comment:3 Changed 7 years ago by Craig Nagy

Owner: set to Craig Nagy
Status: newassigned

Changed 7 years ago by Craig Nagy

Attachment: irc.py.diff added

Changed 7 years ago by Craig Nagy

Attachment: test_irc.py.diff added

Changed 7 years ago by Craig Nagy

Attachment: test_irc.py.2.diff added

Whitespace fixed. Ignore previous diff.

comment:4 Changed 7 years ago by Craig Nagy

Keywords: review added
Owner: Craig Nagy deleted
Status: assignednew

Sorry for the accidental whitespace in the first test_irc.py.diff.

comment:5 Changed 7 years ago by Ying Li

Keywords: review removed

Thank you so much for the work, Nagyman!

Can you please include a news file?

It's basically a short description of what change has been made in this ticket. That way when twisted is released, all these descriptions can be aggregated into the release notes.

That link has the information about description style, as well as where to put the file and what to name the file, etc.

Thanks again!

comment:6 Changed 7 years ago by Ying Li

Sorry, I can't seem to reassign this ticket back to you. :(

Changed 7 years ago by Craig Nagy

Attachment: 3179.diff added

Diff for all changes, now includes a news file.

comment:7 Changed 7 years ago by Craig Nagy

Keywords: review added

New full patch including the required new file. Sorry! I'm not sure why I don't show up in the users list for assignment...I seem to be able to accept ownership myself.

comment:8 Changed 7 years ago by Ying Li

Author: ldleworkcyli, ldlework
Branch: branches/dccchatfactory-protocol-3179

(In [33783]) Branching to 'dccchatfactory-protocol-3179'

comment:9 Changed 7 years ago by Ying Li

(In [33785]) Apply patch from Nagyman

refs #3179

comment:10 Changed 7 years ago by Ying Li

Keywords: review removed

This looks great, Nagyman! Thanks so much for all your hard work.

The build results look good. I'm going to merge.

One small nitpick I have for future patches is that there should be 3 lines between each class and the previous class (see the Twisted coding standard, the section on whitespace).

Once again, thanks!

comment:11 Changed 7 years ago by Ying Li

Resolution: fixed
Status: newclosed

(In [33787]) Merge dccchatfactory-protocol-3179: Return protocol in t.w.protocols.irc.DccChatFactory.buildProtocol

Author: Nagyman Reviewer: cyli Fixes: #3179

Fix bug where the protocol was not returned from twisted.words.protocols.irc.DccChatFactory.buildProtocol

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

Here's some extra feedback about this change:

  1. buildProtocol should most likely have a docstring
  2. Since buildProtocol doesn't use the address passed to it, None might be a better dummy value than "127.0.0.1".
  3. trial provides an assertIsInstance method which provides better information in the failure case than assertTrue(isinstance(...)).

Don't worry about this points now, just keep them in mind for future tickets. Thanks again for your work on this.

Note: See TracTickets for help on using tickets.