Opened 4 years ago

Closed 3 years ago

#4915 defect closed fixed (fixed)

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

Reported by: devinj Owned by:
Priority: normal Milestone:
Component: words Keywords:
Cc: ralphm Branch: branches/irc-service-channelerror-4915
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

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 devinj 4 years ago.
Silly tiny diff.
Twisted-tests.diff (2.3 KB) - added by devinj 4 years ago.
Patch to test cases
Twisted-better.diff (891 bytes) - added by devinj 4 years ago.
Updated patch, now fixes irc_PRIVMSG as well.
Twisted-tests-better.diff (3.9 KB) - added by devinj 4 years ago.
Updated tests, now with a more twisted style and docstrings.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 4 years ago by DefaultCC Plugin

  • Cc ralphm added

Changed 4 years ago by devinj

Silly tiny diff.

Changed 4 years ago by devinj

Patch to test cases

comment:2 Changed 4 years ago by devinj

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 4 years ago by exarkun

  • Owner set to devinj

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 4 years ago by devinj

Updated patch, now fixes irc_PRIVMSG as well.

Changed 4 years ago by devinj

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

comment:4 Changed 4 years ago by devinj

  • 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 4 years ago by exarkun

  • Owner devinj deleted

comment:6 Changed 3 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/irc-service-channelerror-4915

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

comment:7 Changed 3 years ago by exarkun

(In [30923]) Apply patches from the ticket

refs #4915

comment:8 Changed 3 years ago by lvh

  • Keywords review removed

Looks good to me. Merging.

comment:9 Changed 3 years ago by lvh

  • Resolution set to fixed
  • Status changed from new to closed

(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.