Opened 6 years ago

Closed 4 years ago

#5004 enhancement closed fixed (fixed)

replace call to functions from the string module in web

Reported by: Juan A. Diaz Owned by: Richard Wall
Priority: normal Milestone: Python-3.x
Component: web Keywords: py3k
Cc: jknight, Thijs Triemstra, Hynek Schlawack Branch: branches/twisted-web-remove-text-5004-2
branch-diff, diff-cov, branch-cov, buildbot
Author: tomprince, rwall

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 (9)

web.patch (5.7 KB) - added by Juan A. Diaz 6 years ago.
patch
replace-string-module-web-5004.patch (3.0 KB) - added by Jonathan Ballet 4 years ago.
replace-string-module-web-5004.patch-2 (8.2 KB) - added by Jonathan Ballet 4 years ago.
replace-string-module-web-5004-3.patch (8.2 KB) - added by Jonathan Ballet 4 years ago.
replace-string-module-web-5004-4.patch (8.2 KB) - added by Jonathan Ballet 4 years ago.
replace-string-module-web-5004-5.patch (8.2 KB) - added by Jonathan Ballet 4 years ago.
replace-string-module-web-5004-6.patch (9.7 KB) - added by Jonathan Ballet 4 years ago.
replace-string-module-web-5004-7.patch (10.5 KB) - added by Jonathan Ballet 4 years ago.
replace-string-module-web-5004-8.patch (2.1 KB) - added by Jonathan Ballet 4 years ago.
On top of branches/twisted-web-remove-text-5004 [39084]

Download all attachments as: .zip

Change History (46)

comment:1 Changed 6 years ago by DefaultCC Plugin

Cc: jknight added

Changed 6 years ago by Juan A. Diaz

Attachment: web.patch added

patch

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

Summary: replace call to functions from the string module inreplace call to functions from the string module in web

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

Keywords: review removed
Owner: set to Juan A. Diaz

Thanks for working on this. The code changed here (sadly :/) isn't executed by the Twisted Web unit tests. Can you add unit tests that verify this code continues to work properly after this change? Thanks again.

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

My previous review was based on some bogus results from a coverage reporting tool. Actually most of the changes are to code executed by the test suite. However, the change in getSession is not tested. Can you add a test just for that? Thanks again.

comment:5 Changed 6 years ago by Thijs Triemstra

Milestone: Python-3.x

comment:6 Changed 5 years ago by Thijs Triemstra

Cc: Thijs Triemstra added
Owner: Juan A. Diaz deleted

comment:7 Changed 4 years ago by Hynek Schlawack

Cc: Hynek Schlawack added

Changed 4 years ago by Jonathan Ballet

comment:8 Changed 4 years ago by Jonathan Ballet

Keywords: review added
Owner: set to Jonathan Ballet
Status: newassigned

I updated the patch so that all remaining call to functions of the string module has been replace by the equivalent string methods.

If there are still some needs to improve test coverage, I'd gladly do so, what is the process to get the coverage with the Python 3 current scripts?

comment:9 Changed 4 years ago by Thijs Triemstra

Keywords: review removed

Thanks for your patch.

  1. Running pyflakes on twisted.web.server shows types is also unused and can be removed:
    twisted/web/server.py:13: 'string' imported but unused
    twisted/web/server.py:14: 'types' imported but unused
    
  2. only line 91 in twisted.web.twcgi doesn't have coverage, can you add a test for this line?
  3. pyflakes reports an unused import in twisted.web.util that can be removed:
    twisted/web/util.py:18: 'FilePath' imported but unused
    
  4. htmlIndent on line 202 doesn't have test coverage

ps. also un-assign yourself from the ticket next time you add the review keyword, thanks.

comment:10 in reply to:  8 Changed 4 years ago by Thijs Triemstra

Replying to multani:

what is the process to get the coverage with the Python 3 current scripts?

I'm not sure if this documented somewhere, other than the buildbot slave that runs it, but to check coverage I use something like this that creates a html coverage report:

#!/bin/bash

rm -rf _trial_temp
rm -rf twisted-coverage
coverage run --omit /usr/*,_trial_temp/* --branch ./bin/trial --reporter=bwverbose twisted
coverage html -d twisted-coverage --omit /usr/*,_trial_temp/*,Divmod/trunk/* -i

comment:11 Changed 4 years ago by Jean-Paul Calderone

what is the process to get the coverage with the Python 3 current scripts?

I'm not sure if this documented somewhere, other than the buildbot slave that runs it, but to check coverage I use something like this that creates a html coverage report

Note this doesn't generate a coverage report for the code running under Python 3. I'm not sure if coverage.py itself has been ported to Python 3 (I guess it probably has but I haven't checked). The single-codebase approach to porting means that the Python 3 coverage is *usually* pretty similar to the Python 2 coverage though. If you have full Python 2 coverage then you usually don't need to worry about what the Python 3 coverage looks like.

comment:12 in reply to:  9 Changed 4 years ago by Jonathan Ballet

Keywords: review added
Owner: Jonathan Ballet deleted
Status: assignednew

Replying to thijs:

Thanks for your patch.

  1. Running pyflakes on twisted.web.server shows types is also unused and can be removed:
    twisted/web/server.py:13: 'string' imported but unused
    twisted/web/server.py:14: 'types' imported but unused
    

I removed types.

  1. only line 91 in twisted.web.twcgi doesn't have coverage, can you add a test for this line?

I added a test for this line. I had to change a few things in DummyRequest since it wasn't implementing the whole Request interface as needed by CGIScript

  1. pyflakes reports an unused import in twisted.web.util that can be removed:
    twisted/web/util.py:18: 'FilePath' imported but unused
    

This is removed.

  1. htmlIndent on line 202 doesn't have test coverage

I added tests for this as well.

ps. also un-assign yourself from the ticket next time you add the review keyword, thanks.

Sorry about that!

Changed 4 years ago by Jonathan Ballet

comment:13 Changed 4 years ago by Thijs Triemstra

Keywords: review removed
Owner: set to Jonathan Ballet

Thanks for your patch. Some minor things;

  1. It appears you used git diff which produces a different patch format compared to svn diff
  2. 2 blank lines between methods and tests
  3. tests should be named like test_escapeHtml instead of test_escape_html

Changed 4 years ago by Jonathan Ballet

comment:14 in reply to:  13 Changed 4 years ago by Jonathan Ballet

Keywords: review added

Thanks for the review.

Replying to thijs:

Thanks for your patch. Some minor things;

  1. It appears you used git diff which produces a different patch format compared to svn diff

As I did for the first patch. If you are referring to Trac not able to show up the patch correctly against the repository, I would say it's because of the ending extension which is not .patch. I followed http://twistedmatrix.com/trac/wiki/GitMirror in both cases.

  1. 2 blank lines between methods and tests

Done.

  1. tests should be named like test_escapeHtml instead of test_escape_html

Done.

Changed 4 years ago by Jonathan Ballet

comment:15 Changed 4 years ago by therve

Owner: Jonathan Ballet deleted

comment:16 Changed 4 years ago by lvh

Owner: set to lvh

comment:17 Changed 4 years ago by lvh

Keywords: review removed
Owner: changed from lvh to Jonathan Ballet

Hi multani! Thanks for working on this ticket.

Looks mostly good! Two questions from the diff (I get line numbers from https://twistedmatrix.com/trac/attachment/ticket/5004/replace-string-module-web-5004-4.patch):

twisted/web/twcgi.py, line 53 in the new file, and later 58, 59: why doesn't this just do _reactor=reactor? It seems reactor behaves as the default value here.

twisted/web/twcgi.py, line 104 in the new file uses str.find; maybe it's a good idea to turn this into str.index and replace the if/else with try/except since str.find is pretty terrible (it works now, because it explicitly checks for the (dumb) return value of str.find, but I think str.index would be an improvement).

Otherwise, LGTM.

Thanks again for working on this lvh

comment:18 Changed 4 years ago by Jean-Paul Calderone

I like the idiom:

def foo(name=None):
    if name is None:
        name = default
    ...

In particular, it makes it easier for callers to invoke the default behavior. It's easier and safer to pass None than to hard-code the actual default value of a parameter at the call-site (or write two code-paths, one which passes a value for a parameter if you have a good value to pass, one that doesn't if you don't).

In any case, the coding standard doesn't call for one or the other, so I think this part of the patch is fine.

Changed 4 years ago by Jonathan Ballet

comment:19 in reply to:  17 Changed 4 years ago by Jonathan Ballet

Keywords: review added
Owner: Jonathan Ballet deleted

Replying to lvh:

Hi multani! Thanks for working on this ticket.

Thanks for the review!

Beside your reivew, I also rebased the patch on the latest trunk.

Looks mostly good! Two questions from the diff (I get line numbers from https://twistedmatrix.com/trac/attachment/ticket/5004/replace-string-module-web-5004-4.patch):

twisted/web/twcgi.py, line 53 in the new file, and later 58, 59: why doesn't this just do _reactor=reactor? It seems reactor behaves as the default value here.

I tend to agree with exarkun on this. I'm not very opiniated though, so I'm still willing to change it if it helps to get this merged.

twisted/web/twcgi.py, line 104 in the new file uses str.find; maybe it's a good idea to turn this into str.index and replace the if/else with try/except since str.find is pretty terrible (it works now, because it explicitly checks for the (dumb) return value of str.find, but I think str.index would be an improvement).

Yeah, .index is a nice improvement, so this is now done.

(FYI, it's also available as a Git branch at https://github.com/multani/twisted/tree/replace-string-module-web-5004)

comment:20 Changed 4 years ago by Tom Prince

Author: tomprince
Branch: branches/twisted-web-remove-text-5004

(In [38927]) Branching to twisted-web-remove-text-5004.

comment:21 Changed 4 years ago by Tom Prince

(In [38928]) Apply replace-string-module-web-5004-5.patch from multani.

Refs: #5004

comment:22 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Jonathan Ballet
  1. twcgi and the reactor:
    1. Persuant to this mailing list thread, please make both the argument and attribute public.
    2. FilteredScript should also use the specified reactor (this will need a test)
    3. Since the default value is the only place the global reactor will be used, please import it conditionally if required. This means that importing this module won't (directly) import the reactor (thus installing the default).
  2. I started a build.

Other than that, this look good. Please resubmit, with a patch against the branch.

comment:23 in reply to:  22 ; Changed 4 years ago by Jonathan Ballet

Replying to tom.prince:

  1. twcgi and the reactor:
    1. Persuant to this mailing list thread, please make both the argument and attribute public.

I understand for the argument, but should I really make the attribute public as well?

comment:24 in reply to:  23 Changed 4 years ago by Tom Prince

Replying to multani:

  1. twcgi and the reactor:
    1. Persuant to this mailing list thread, please make both the argument and attribute public.

I understand for the argument, but should I really make the attribute public as well?

I think I had a reasonable motivation for suggesting that, when I wrote that. But reflecting on it, I can't think of a good reason for it. Furthermore, it is easy to change it at a later date to make it public, but not the reverse. So erring on the side of making it private seems reasonable.

Changed 4 years ago by Jonathan Ballet

comment:25 in reply to:  22 Changed 4 years ago by Jonathan Ballet

Keywords: review added

Replying to tom.prince:

  1. twcgi and the reactor:
    1. Persuant to this mailing list thread, please make both the argument and attribute public.

Done.

  1. FilteredScript should also use the specified reactor (this will need a test)

Done. I had to change slightly the test's tear down, which supposed the custom startServer method was called (which I don't for my tests).

Actually, I guess those custom tests method could go away, since we can now passes a custom reactor to FilteredScript instead of starting a real one.

  1. Since the default value is the only place the global reactor will be used, please import it conditionally if required. This means that importing this module won't (directly) import the reactor (thus installing the default).

Done.

  1. I started a build.

Other than that, this look good. Please resubmit, with a patch against the branch.

I rebased the patch against latest trunk.

comment:26 Changed 4 years ago by Tom Prince

(In [39081]) Apply replace-string-module-web-5004-6.patch from multani.

Refs: #5004

comment:27 Changed 4 years ago by Tom Prince

Keywords: review removed

Thanks for your continued work on this.

  1. Please add docstrings to the methods added to DummyRequest.
    • When looking at them, I noticed that getHost returns a transport. But IRequest.getHost returns an IAddress, not a transport. Please change it to return an IAddress (probably an IPv4Address).
  2. I fixed a few twistedchecker errors: some whitespace fixes, and s/_fake_reactor/fakeReactor/.
  3. I reverted the change back of index back to find.

Please resubmit for review after addressing 1 with a patch against the branch.

Changed 4 years ago by Jonathan Ballet

comment:28 in reply to:  27 Changed 4 years ago by Jonathan Ballet

Keywords: review added

Sorry it took so long for just a few docstrings...

Replying to tom.prince:

Thanks for your continued work on this.

  1. Please add docstrings to the methods added to DummyRequest.
    • When looking at them, I noticed that getHost returns a transport. But IRequest.getHost returns an IAddress, not a transport. Please change it to return an IAddress (probably an IPv4Address).

I added the docstrings and fixed the returned value of getHost. I wasn't so much inspired for the docstrings, so they are merely based on what twisted.web.http.Request presents.

comment:29 Changed 4 years ago by hawkowl

Owner: changed from Jonathan Ballet to hawkowl

comment:30 Changed 4 years ago by hawkowl

Keywords: review removed
Owner: changed from hawkowl to Jonathan Ballet

Hi multani! Thanks for resubmitting.

  1. Should "L{htmlIndent} transparently process input with no special cases inside." have "processes" instead of "process"? (changing this will require reflowing the docstring)
  2. Should "L{htmlIndent} replaces tab characters by unbreakable spaces." have "with" rather than "by"?
  3. Maybe "Retrieve all the values of the request headers as a dictionary." should use "get" to fall in line with the rest of the docstrings, as well as the function name?

Otherwise, I don't see anything obviously wrong with it, and I don't see any code problems that the above haven't fixed - please fix these and resubmit.

Changed 4 years ago by Jonathan Ballet

On top of branches/twisted-web-remove-text-5004 [39084]

comment:31 Changed 4 years ago by Jonathan Ballet

Keywords: review added

Hi hawkowl, thanks for the review.

I'm proposing a new patch on top of the branch proposed by Tom in comment:26 which fixes your two first points. I let "Retrieve" instead of "Get" since this was also the term used by the previous getHeader` method.

comment:32 Changed 4 years ago by Richard Wall

Owner: changed from Jonathan Ballet to Richard Wall
Status: newassigned

Reviewing...

comment:33 Changed 4 years ago by Richard Wall

(In [40816]) Apply replace-string-module-web-5004-8.patch from multani. Refs #5004

comment:34 Changed 4 years ago by Richard Wall

Keywords: review removed
Status: assignednew

Thanks multani,

This looks pretty good to me. Just a few nit picks which I've listed below.

Notes:

Points:

  1. Missing newsfile -- I'll add one.
    1. https://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles
  2. Some twistedchecker warnings
    1. https://buildbot.twistedmatrix.com/builders/twistedchecker/builds/1536/steps/run-twistedchecker/logs/new%20twistedchecker%20errors
  3. There is some missing coverage adjacent to some code you changed, but none of the other reviewers noted it, so I think we can ignore it.
    1. https://buildbot.twistedmatrix.com/builds/twisted-coverage.py/twisted-coverage.py-r40816/twisted_web_twcgi.html#n107
  4. source:branches/twisted-web-remove-text-5004/twisted/web/util.py
    1. Non-standard line wrapping on 201, 202
      1. See https://twistedmatrix.com/documents/current/core/development/policy/coding-standard.html#auto5
  5. source:branches/twisted-web-remove-text-5004/twisted/web/test/requesthelper.py
    1. getAllHeaders: Missing @return documentation
    2. getClient: Unnecessary "pass" statement
  6. source:branches/twisted-web-remove-text-5004/twisted/web/test/test_cgi.py
    1. test_useReactorArgument: Missing docstrings for FakeReactor
    2. def test_pathInfo: the assertIn test for the presence of the key looks unnecessary, since you're testing the value of that key later anyway.

I think I'll go ahead and fix those and merge if the build results come back green.

Thanks again to everyone who's worked on this ticket.

-RichardW.

comment:35 Changed 4 years ago by Richard Wall

Author: tomprincetomprince, rwall
Branch: branches/twisted-web-remove-text-5004branches/twisted-web-remove-text-5004-2

(In [40817]) Branching to 'twisted-web-remove-text-5004-2'

comment:36 Changed 4 years ago by Richard Wall

Addressed code review comments:

  • log:branches/twisted-web-remove-text-5004-2

Build results:

comment:37 Changed 4 years ago by Richard Wall

Resolution: fixed
Status: newclosed

(In [40826]) Merge twisted-web-remove-text-5004-2

Author: nueces, multani Reviewers: exarkun, thijs, lvh, tom.prince, hawkowl, rwall Fixes: #5004

Replace all use of the string module in twisted.web, with the equivalent string methods.

Note: See TracTickets for help on using tickets.