Ticket #4915 defect closed fixed

Opened 2 years ago

Last modified 2 years ago

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

Twisted.diff Download (0.5 KB) - added by devinj 2 years ago.
Silly tiny diff.
Twisted-tests.diff Download (2.3 KB) - added by devinj 2 years ago.
Patch to test cases
Twisted-better.diff Download (0.9 KB) - added by devinj 2 years ago.
Updated patch, now fixes irc_PRIVMSG as well.
Twisted-tests-better.diff Download (3.9 KB) - added by devinj 2 years ago.
Updated tests, now with a more twisted style and docstrings.

Change History

1

Changed 2 years ago by DefaultCC Plugin

  • cc ralphm added

Changed 2 years ago by devinj

Silly tiny diff.

Changed 2 years ago by devinj

Patch to test cases

2

Changed 2 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.

3

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

Updated patch, now fixes irc_PRIVMSG as well.

Changed 2 years ago by devinj

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

4

Changed 2 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!

5

Changed 2 years ago by exarkun

  • owner devinj deleted

6

Changed 2 years ago by exarkun

  • branch set to branches/irc-service-channelerror-4915
  • branch_author set to exarkun

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

7

Changed 2 years ago by exarkun

(In [30923]) Apply patches from the ticket

refs #4915

8

Changed 2 years ago by lvh

  • keywords review removed

Looks good to me. Merging.

9

Changed 2 years ago by lvh

  • status changed from new to closed
  • resolution set to fixed

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