Opened 6 years ago

Closed 4 years ago

Last modified 4 years ago

#5329 enhancement closed fixed (fixed)

Replace deprecated string functions with str methods in t.w.p.irc

Reported by: Ezio Melotti Owned by: Thijs Triemstra
Priority: low Milestone:
Component: words Keywords: irc
Cc: ralphm, Thijs Triemstra Branch: branches/replace-irc-string-5329-2
branch-diff, diff-cov, branch-cov, buildbot
Author: thijs, Wolf

Description

The attached patch replaces deprecated function from the string module with the corresponding str methods in t.w.p.irc.

Attachments (4)

deprecated_string_functions.diff (9.9 KB) - added by Ezio Melotti 6 years ago.
string-2.diff (16.5 KB) - added by Ezio Melotti 5 years ago.
issue5329.diff (18.5 KB) - added by Ezio Melotti 4 years ago.
issue5329-2.diff (19.8 KB) - added by Ezio Melotti 4 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 6 years ago by DefaultCC Plugin

Cc: ralphm added

Changed 6 years ago by Ezio Melotti

comment:2 Changed 6 years ago by Ezio Melotti

Keywords: irc noticed review added

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

Keywords: review removed
Owner: set to Ezio Melotti

Thanks for your work on this. A lot of the changed code is untested. Do you think you could add some unit tests to exercise it?

comment:4 Changed 5 years ago by Ezio Melotti

Keywords: review added
Owner: changed from Ezio Melotti to Jean-Paul Calderone

Attached patch adds tests for most of the changes. A few changes are still untested. The patch includes a test client that collects in a list the names of the methods and the arguments passed, and two classes to test the irc_* and ctcpQuery_* methods. All the tests pass with and without the changes to t.w.p.irc.

Changed 5 years ago by Ezio Melotti

Attachment: string-2.diff added

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

Owner: Jean-Paul Calderone deleted

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

Keywords: review removed
Owner: set to Ezio Melotti

Thanks. The tests need to conform to the testing standard, ie they need docstrings, they need to be formatted appropriately, etc.

Changed 4 years ago by Ezio Melotti

Attachment: issue5329.diff added

comment:7 Changed 4 years ago by Ezio Melotti

Keywords: review added
Owner: Ezio Melotti deleted

I now added the docstrings to the tests.

comment:8 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Ezio Melotti

Thanks for your contribution, particularly the test. A couple of minor points:

  1. Untested changes:
    • sendMessage
    • dccDescribe
      • Also, python 1.5 is no longer supported, so I think the 'map(int, ...)' can go as well.
  2. badMessage should probably log.err with a twisted.python.Failure constructed from its arguments.
  3. (minor) The class docstrings for tests should have """ on separate lines. And there should be two blank lines between methods.
  4. The tests use self.patch for modifying an instance. There is no reason to do that, since it will be destroyed at the the end of the test anyway.

Please resubmit for review after addressing the above points.

Changed 4 years ago by Ezio Melotti

Attachment: issue5329-2.diff added

comment:9 Changed 4 years ago by Ezio Melotti

Keywords: review added
Owner: Ezio Melotti deleted

I addressed the comments and attached a new patch.

comment:10 Changed 4 years ago by Thijs Triemstra

Author: thijs
Branch: branches/replace-irc-string-5329

(In [38286]) Branching to 'replace-irc-string-5329'

comment:11 Changed 4 years ago by Thijs Triemstra

(In [38287]) apply issue5329-2.diff, add news file. refs #5329

comment:12 Changed 4 years ago by Thijs Triemstra

Author: thijsWolf
Cc: Thijs Triemstra added
Keywords: review removed
Owner: set to Ezio Melotti

Thanks for your patch Wolf and for keeping this ticket active for 18 months now, looks like we're almost there :)

In twisted/words/test/test_irc.py:

  1. test_sendMessage:
    1. Thanks for testing sendMessage. could you also add a simple test for the 2 cases that a ValueError is raised? Should be very straightforward.
    2. The docstring should be more descriptive, what kind of sanity check is this?
  2. CollectorClient:
    1. methods_list should be documented using @param and @type instead of documenting it in an inline comment.
  3. TestCTCPQuery:
    1. Many of the tests have a docstring that starts with 'Test that', but you should avoid this (because we already know its a test/testing) and describe only the behavior or outcome of the test.
  4. dccDescribeTests:
    1. This class name should start with a capital
    2. Same comment about the docstring of the test as in comment 3.1.

In twisted/words/protocols/irc.py:

  1. line 1772 is untested
  2. line 2396 is untested. You also change the error message here, why is that? This kind of new feature would have to be addressed in a separate ticket.
  3. line 2662 is untested
  4. the comment above line 2726 can go

I put your patch in a branch and forced a new build. Please supply future patches against that branch, thanks.

comment:13 in reply to:  12 Changed 4 years ago by Thijs Triemstra

Replying to thijs:

In twisted/words/protocols/irc.py:

  1. line 1772 is untested
  2. line 2396 is untested. You also change the error message here, why is that? This kind of new feature would have to be addressed in a separate ticket.

I now see that tom prince suggested this change earlier, so ignore my comment except for the fact that this line should be tested as well, thanks.

comment:14 Changed 4 years ago by Thijs Triemstra

Author: Wolfthijs, Wolf
Branch: branches/replace-irc-string-5329branches/replace-irc-string-5329-2

(In [38938]) Branching to 'replace-irc-string-5329-2'

comment:15 Changed 4 years ago by Thijs Triemstra

(In [38941]) address review comments, refs #5329

comment:16 in reply to:  12 Changed 4 years ago by Thijs Triemstra

Keywords: review added
Owner: Ezio Melotti deleted

Merged the branch forward and addressed most review comments, see below. Forced a build and fixed the twistedchecker errors.

Replying to tom.prince:

  1. badMessage should probably log.err with a twisted.python.Failure constructed from its arguments.

Can this be a new ticket?

Replying to thijs:

In twisted/words/test/test_irc.py:

  1. test_sendMessage:
    1. Thanks for testing sendMessage. could you also add a simple test for the 2 cases that a ValueError is raised? Should be very straightforward.

Added two additional tests.

  1. The docstring should be more descriptive, what kind of sanity check is this?

Fixed.

  1. CollectorClient:
    1. methods_list should be documented using @param and @type instead of documenting it in an inline comment.

Fixed by merging forward.

  1. TestCTCPQuery:
    1. Many of the tests have a docstring that starts with 'Test that', but you should avoid this (because we already know its a test/testing) and describe only the behavior or outcome of the test.

Fixed.

  1. dccDescribeTests:
    1. This class name should start with a capital

Fixed.

In twisted/words/protocols/irc.py:

  1. line 2396 is untested. You also change the error message here, why is that? This kind of new feature would have to be addressed in a separate ticket.

Reverted the change.

comment:17 Changed 4 years ago by Tom Prince

Keywords: noticed review removed
Owner: set to Thijs Triemstra

Thanks for working on this

  1. test_sendMessage: Words like "correct" and "valid" should be avoided in test docstrings, the docstring should say what it means for the query string to be valid (see #6301)
    • The same is true for TestCTCPQuery
  1. badMessage should probably log.err with a twisted.python.Failure constructed from its arguments.

Can this be a new ticket?

Sure.

  • Since it will be addressed in a seperate ticket, we should consider deprecating badMessage, and replacing it with a function that takes a Failure instead.

Please merge after addressing 1, and filing a ticket for 2.

comment:18 Changed 4 years ago by Thijs Triemstra

(In [38978]) address review comments, refs #5329

comment:19 in reply to:  17 Changed 4 years ago by Thijs Triemstra

Status: newassigned

Thanks for the review.

Replying to tom.prince:

Please merge after addressing 1, and filing a ticket for 2.

Opened #6605.

comment:20 Changed 4 years ago by Thijs Triemstra

Resolution: fixed
Status: assignedclosed

(In [38979]) Merge replace-irc-string-5329-2: Replaced usage of deprecated string functions in words.protocols.irc.

Author: Wolf, thijs Reviewer: exarkun, tom.prince Fixes: #5329

comment:21 Changed 4 years ago by Ezio Melotti

Thanks!

Note: See TracTickets for help on using tickets.