Opened 3 years ago

Closed 2 years ago

#4999 enhancement closed fixed (fixed)

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

Reported by: nueces Owned by: thijs
Priority: normal Milestone: Python-3.x
Component: core Keywords: py3k
Cc: Branch: branches/str-methods-python-4999-2
(diff, github, buildbot, log)
Author: nueces, thijs Launchpad Bug:

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 nueces 3 years ago.
patch

Download all attachments as: .zip

Change History (21)

Changed 3 years ago by nueces

patch

comment:1 Changed 3 years ago by nueces

  • Keywords changed from py3k, review to py3k review

comment:2 Changed 3 years ago by lvh

  • Author set to lvh
  • Branch set to branches/str-methods-python-4999

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

comment:3 Changed 3 years ago by lvh

  • Owner set to lvh
  • Status changed from new to assigned

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

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

comment:4 Changed 3 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 3 years ago by exarkun

This patch modifies untested code.

comment:6 Changed 3 years ago by lvh

  • Owner changed from lvh to nueces
  • Status changed from assigned to new

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 3 years ago by thijs

  • Milestone set to Python-3.x

comment:8 Changed 2 years ago by thijs

  • Owner nueces deleted

comment:9 Changed 2 years ago by thijs

  • Author changed from lvh to thijs, lvh
  • Branch changed from branches/str-methods-python-4999 to branches/str-methods-python-4999-2

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

comment:10 Changed 2 years ago by thijs

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

comment:11 Changed 2 years ago by thijs

  • Author changed from thijs, lvh to nueces, 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 2 years ago by vperic

  • Keywords review removed
  • Owner set to thijs

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

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

comment:14 Changed 2 years ago by thijs

  • Keywords review added
  • Owner thijs 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 2 years ago by itamar

  • Keywords review removed
  • Owner set to thijs
  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 2 years ago by vperic

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 follow-up: Changed 2 years ago by 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.

comment:18 Changed 2 years ago by thijs

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

comment:19 in reply to: ↑ 17 Changed 2 years ago by thijs

  • Status changed from new to assigned

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

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

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