Opened 5 years ago

Closed 5 years ago

#3910 defect closed fixed (fixed)

twisted.words.protocols.irc: Don't prepend # onto channel name in IRCClient.me

Reported by: stefanor Owned by:
Priority: normal Milestone:
Component: words Keywords:
Cc: Branch:
Author: Launchpad Bug:

Description

You can't send an action privately (not in a channel) using IRCClient, because IRCClient.me prepends a # onto the channel name. While that behaviour is documented in docstrings elsewhere (See ticket #3895), it is a bad API choice.

Without this patch, the alternative is to call IRCClient.ctcpMakeQuery directly.

Attachments (2)

t.w.p.irc-action.patch (2.5 KB) - added by stefanor 5 years ago.
describe() function replaces me() and doesn't prepend #
t.w.p.irc-action-2.patch (3.7 KB) - added by stefanor 5 years ago.
Improve tests and test deprecation

Download all attachments as: .zip

Change History (10)

comment:1 follow-up: Changed 5 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to stefanor

This is what the API does. Changing it may break existing software which relies on it.

Instead, a new method with the desired behavior should be introduced, and possibly the existing method should be deprecated.

Please be sure to include unit tests along with any change. :)

Changed 5 years ago by stefanor

describe() function replaces me() and doesn't prepend #

comment:2 in reply to: ↑ 1 Changed 5 years ago by stefanor

  • Keywords review added

Replying to exarkun:

Instead, a new method with the desired behavior should be introduced, and possibly the existing method should be deprecated.

All the good names are taken. the best I can come up with is describe()

Please be sure to include unit tests along with any change. :)

Bleh, OK.

comment:3 Changed 5 years ago by stefanor

  • Owner changed from stefanor to exarkun

comment:4 follow-up: Changed 5 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to stefanor

Thanks for the new patch. I just have a few comments:

  1. The new test method, test_describe, should have a docstring describing what behavior it covers. You can see examples of such docstrings just above it on the other test methods in that class.
  2. The Twisted coding standard calls for two blank lines between method definitions; please add a blank line before test_describe and after describe.
  3. The me deprecation should also be unit tested. Unfortunately this is pretty old code, so me doesn't have any existing unit tests. Also unfortunately, Twisted hasn't quite settled on a convention for testing deprecations, so there are a few possibilities for you to choose from here. :) One style involves TestCase.flushWarnings, you can find an example of this style in test_util.py. Another style involves TestCase.callDeprecated, and you can find an example in test_keys.py (this involves a different implementation of emitting the warning too, see asn1.py. There's also TestCase.assertWarns, but either of the previous forms is probably preferable to that.
  4. The deprecation should mention that the API was first deprecated in Twisted 9.0. Depending on how you resolve the previous point, this may be fixed automatically.
  5. The new describe method should include the epytext markup @since: 9.0 in its docstring.

That's all. Thanks again for working on this.

Changed 5 years ago by stefanor

Improve tests and test deprecation

comment:5 in reply to: ↑ 4 Changed 5 years ago by stefanor

  • Keywords review added
  • Owner changed from stefanor to exarkun

Replying to exarkun:

Thanks again for working on this.

OK, I admit I was lazier in the earlier patch attempts :P

Hope this solves all the issues.

comment:6 Changed 5 years ago by exarkun

  • Keywords review removed

Thanks, looks great. I'll apply it.

comment:7 Changed 5 years ago by exarkun

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

(In [27131]) Apply t.w.p.irc-action-2.patch adding a better API than IRCClient.me

Author: stefanor
Reviewer: exarkun
Fixes: #3910

Add IRCClient.describe, a replacement for IRCClient.me which doesn't do
confusing, unhelpful target mangling. Also deprecate IRCClient.me.

comment:8 Changed 3 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.