Opened 7 years ago

Closed 7 years ago

#4915 defect closed fixed (fixed)

twisted.words.services references nonexistent twisted.words.protocols.irc.IRC_NOSUCHCHANNEL

Reported by: Devin Jeanpierre Owned by:
Priority: normal Milestone:
Component: words Keywords:
Cc: ralphm Branch: branches/irc-service-channelerror-4915
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description

http://twistedmatrix.com/trac/browser/trunk/twisted/words/service.py#L549

It meant to say ERR_NOSUCHCHANNEL, as seen later in the same method.

Attachments (4)

Twisted.diff (552 bytes) - added by Devin Jeanpierre 7 years ago.
Silly tiny diff.
Twisted-tests.diff (2.3 KB) - added by Devin Jeanpierre 7 years ago.
Patch to test cases
Twisted-better.diff (891 bytes) - added by Devin Jeanpierre 7 years ago.
Updated patch, now fixes irc_PRIVMSG as well.
Twisted-tests-better.diff (3.9 KB) - added by Devin Jeanpierre 7 years ago.
Updated tests, now with a more twisted style and docstrings.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 7 years ago by DefaultCC Plugin

Cc: ralphm added

Changed 7 years ago by Devin Jeanpierre

Attachment: Twisted.diff added

Silly tiny diff.

Changed 7 years ago by Devin Jeanpierre

Attachment: Twisted-tests.diff added

Patch to test cases

comment:2 Changed 7 years ago by Devin Jeanpierre

The attached test cases test whether the IRC server sends the expected response in all the cases where invalid UTF-8 is sent for text data. Other than the case described in this ticket, there's another case that fails, but I don't know what it's supposed to do. That case is irc_PRIVMSG:

If the passed targetName is not valid UTF-8, it apparently wants to send that targetName as part of the response. But the targetName couldn't be decoded. This leads to an UnboundLocalError.

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

Owner: set to Devin Jeanpierre

Hi devinj,

Thanks for the patch! In the future, make sure you add the "review" keyword to the ticket so someone is certain to notice. :)

Some comments about the patch:

  1. All classes and methods should have docstrings. For test classes and methods, the docstring needs to explain the behavior being verified.
  2. Non-test methods should generally not have an underscore - in Twisted, underscore is reserved for separating a prefix from a dynamic suffix for dynamic dispatch. So assert_chokesOnBadBytes should be assertChokesOnBadBytes. _mocked_codes should be mockedCodes.
  3. test_PRIVMSG is still failing, can you fix it as well?

Thanks again for helping out here. It's really great to have more eyes on twisted.words :)

Changed 7 years ago by Devin Jeanpierre

Attachment: Twisted-better.diff added

Updated patch, now fixes irc_PRIVMSG as well.

Changed 7 years ago by Devin Jeanpierre

Attachment: Twisted-tests-better.diff added

Updated tests, now with a more twisted style and docstrings.

comment:4 Changed 7 years ago by Devin Jeanpierre

Keywords: review added

I updated the tests per your suggestions, and I also fixed test_PRIVMSG. Turns out it was really easy, I just didn't give it more than a glance. The new patches contain all the changes in the old patches, plus the requested additional changes.

I'm not sure if more description is warranted in the tests. Let me know if more rationale-type explanation should be added, and I'll do my best!

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

Owner: Devin Jeanpierre deleted

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

Author: exarkun
Branch: branches/irc-service-channelerror-4915

(In [30922]) Branching to 'irc-service-channelerror-4915'

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

(In [30923]) Apply patches from the ticket

refs #4915

comment:8 Changed 7 years ago by lvh

Keywords: review removed

Looks good to me. Merging.

comment:9 Changed 7 years ago by lvh

Resolution: fixed
Status: newclosed

(In [31007]) twisted.words.services no longer references nonexistent twisted.words.protocols.irc.IRC_NOSUCHCHANNEL.

Author: devinj Reviewer: lvh, exarkun Fixes: #4915

twisted.words.services is no longer fundamentally wrong, and a bunch of test got added in the related code. Yay!

Note: See TracTickets for help on using tickets.