Ticket #3910 defect closed fixed

Opened 5 years ago

Last modified 5 years ago

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

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

Change History

1

follow-up: ↓ 2   Changed 5 years ago by exarkun

  • owner changed from exarkun to stefanor
  • keywords review removed

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 #

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.

3

  Changed 5 years ago by stefanor

  • owner changed from stefanor to exarkun

4

follow-up: ↓ 5   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

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.

6

  Changed 5 years ago by exarkun

  • keywords review removed

Thanks, looks great. I'll apply it.

7

  Changed 5 years ago by exarkun

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

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

8

  Changed 3 years ago by <automation>

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