Opened 6 years ago

Closed 5 years ago

#4999 enhancement closed fixed (fixed)

replace call to functions from the string module in the python module

Reported by: Juan A. Diaz Owned by: Thijs Triemstra
Priority: normal Milestone: Python-3.x
Component: core Keywords: py3k
Cc: Branch: branches/str-methods-python-4999-2
branch-diff, diff-cov, branch-cov, buildbot
Author: nueces, thijs

Description

To make the port to python 3k more easy, I'm replacing all the calls to functions from the string module, for their equivalent from the str type object or calling directly to the built-in method of the string variable.

Attachments (1)

python.patch (5.4 KB) - added by Juan A. Diaz 6 years ago.
patch

Download all attachments as: .zip

Change History (21)

Changed 6 years ago by Juan A. Diaz

Attachment: python.patch added

patch

comment:1 Changed 6 years ago by Juan A. Diaz

Keywords: py3k, reviewpy3k review

comment:2 Changed 6 years ago by lvh

Author: lvh
Branch: branches/str-methods-python-4999

(In [31458]) Branching to 'str-methods-python-4999'

comment:3 Changed 6 years ago by lvh

Owner: set to lvh
Status: newassigned

Patch looks clean, waiting for buildbot to come back clean:

http://twistedmatrix.com/trac/raw-attachment/ticket/4999/python.patch

comment:4 Changed 6 years ago by lvh

Keywords: review removed

Whoops! That was obviously the wrong link. I meant this:

http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/str-methods-python-4999

... which looks clean except for an unrelated IOCP failure.

Added to my merge queue.

Thanks as always for your patch!

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

This patch modifies untested code.

comment:6 Changed 6 years ago by lvh

Owner: changed from lvh to Juan A. Diaz
Status: assignednew

What JP said. If you don't feel like adding a test, please unassign yourself (== assign to the empty item at the top of the list), by the way :)

comment:7 Changed 5 years ago by Thijs Triemstra

Milestone: Python-3.x

comment:8 Changed 5 years ago by Thijs Triemstra

Owner: Juan A. Diaz deleted

comment:9 Changed 5 years ago by Thijs Triemstra

Author: lvhthijs, lvh
Branch: branches/str-methods-python-4999branches/str-methods-python-4999-2

(In [35057]) Branching to 'str-methods-python-4999-2'

comment:10 Changed 5 years ago by Thijs Triemstra

(In [35059]) replace string methods in t.python, refs #4999

comment:11 Changed 5 years ago by Thijs Triemstra

Author: thijs, lvhnueces, thijs
Keywords: review added

Replaced the string methos and added some new tests for the code that was changed but not covered. Build results.

comment:12 Changed 5 years ago by Vladimir Perić

Keywords: review removed
Owner: set to Thijs Triemstra

Hmm, this overlaps with my patch at #5830 (make test_text.py pass in py3). As far as that module is concerne, the changes are the same (though I had some others, unrelated to the string module) but the tests are different. In general, I think your tests are a bit better - the problem is in test_dict - the ordering of elements in a dictionary is not cannonical so I believe it *will* fail on some architecture combination. In my patch, I tested just a single element dict, which will always work but provides less coverage (though it did cover all the changed lines so it was technically correct). Also, in the LineTests class, you might want to consider adding some assertFalse's, though I don't think it's really necessary.

Another note, (and I've been bitten by this myself), apparently Trac separates keywords with spaces, not commas, so you should use "py3k review" instead of "py3k, review". I'm also not sure what's the point of the keyword, especially when we have the milestone, so I haven't even been using it lately.

Anyway, please fix test_dict and then I'll check if all changed lines are covered (though I think they are). Consider this an official review. :)

comment:13 Changed 5 years ago by Thijs Triemstra

(In [35080]) address review comments. refs #4999

comment:14 Changed 5 years ago by Thijs Triemstra

Keywords: review added
Owner: Thijs Triemstra deleted

Thanks for the review. It's unfortunate the tickets do slightly overlap but the part that was changed for #5830 related to the string method was designated for this ticket (and maybe should've been fixed first, which were about to do :) Landing this ticket will only make the other ticket easier to review and fix.

Build results.

comment:15 Changed 5 years ago by Itamar Turner-Trauring

Keywords: review removed
Owner: set to Thijs Triemstra
  1. Drop the changes to twisted.python.hook, they're unrelated to this branch.
  2. LineTest needs a docstring.
  3. In docstrings of test methods, rather than saying "X supports Y", you should explain what X's behavior is. What is stringyString supposed to do to tuples?

Other than that it looks good, so once above are fixed and buildbot is happy it should be merged.

comment:16 Changed 5 years ago by Vladimir Perić

I think you misunderstood my comment about test_dict - any dict with more than 1 key-value pair (and it now has two) can be a problem. I'm referring to this particular line from the documentation:

"CPython implementation detail: Keys and values are listed in an arbitrary order which is non-random, varies across Python implementations, and depends on the dictionary’s history of insertions and deletions."

comment:17 Changed 5 years ago by Itamar Turner-Trauring

Ah, yes. You shouldn't assume any particular order of dictionary items, as test_dict currently does. So that'd need to be fixed.

comment:18 Changed 5 years ago by Thijs Triemstra

(In [35147]) address review comments, refs #4999

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

Status: newassigned

Replying to itamar:

Ah, yes. You shouldn't assume any particular order of dictionary items, as test_dict currently does. So that'd need to be fixed.

Fixed.

Replying to itamar:

  1. Drop the changes to twisted.python.hook, they're unrelated to this branch.

I reverted the coding standard stuff and left the string import change in there

  1. LineTest needs a docstring.

Fixed.

  1. In docstrings of test methods, rather than saying "X supports Y", you should explain what X's behavior is. What is stringyString supposed to do to tuples?

Fixed.

Other than that it looks good, so once above are fixed and buildbot is happy it should be merged.

Build results.

comment:20 Changed 5 years ago by Thijs Triemstra

Resolution: fixed
Status: assignedclosed

(In [35152]) Merge str-methods-python-4999-2: Replaced calls to functions from the string module in twisted.python.

Author: nueces, thijs Reviewer: lvh, exarkun, vperic, itamar Fixes: #4999

Note: See TracTickets for help on using tickets.