Opened 3 years ago

Closed 3 years ago

#7845 enhancement closed fixed (fixed)

Make twisted.web.error compatible with Python 3

Reported by: Dustin J. Mitchell Owned by: hawkowl
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/py3-twisted-web-error-7845-2
branch-diff, diff-cov, branch-cov, buildbot
Author: hawkowl, dustin, glyph

Description

I thought this would be easy, but it has a nefarious dependency on twisted.web.template which required a lot of almost modules.

Attachments (1)

py3-twisted-web-error.patch (26.6 KB) - added by Dustin J. Mitchell 3 years ago.
py3-twisted-web-error.patch

Download all attachments as: .zip

Change History (30)

Changed 3 years ago by Dustin J. Mitchell

Attachment: py3-twisted-web-error.patch added

py3-twisted-web-error.patch

comment:1 Changed 3 years ago by Adi Roiban

Author: adiroiban
Branch: branches/twisted.web.error-py3-7845

(In [44457]) Branching to twisted.web.error-py3-7845.

comment:2 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: set to Dustin J. Mitchell

I tried to apply the patch on trunk but it failed

$ patch -p1 < ~/Downloads/py3-twisted-web-error.patch 
patching file twisted/python/compat.py
Hunk #1 FAILED at 11.
Hunk #2 succeeded at 384 (offset 81 lines).
1 out of 2 hunks FAILED -- saving rejects to file twisted/python/compat.py.rej
patching file twisted/python/dist3.py
Hunk #1 FAILED at 216.
Hunk #2 succeeded at 277 (offset 32 lines).
1 out of 2 hunks FAILED -- saving rejects to file twisted/python/dist3.py.rej
patching file twisted/python/urlpath.py
patching file twisted/web/_element.py
patching file twisted/web/_flatten.py
patching file twisted/web/_responses.py
patching file twisted/web/error.py
patching file twisted/web/template.py
patching file twisted/web/test/test_error.py
patching file twisted/web/test/test_web__responses.py

Can you please take a look at the patch.

I am not a big fan of diffs attached to Trac so I would be happy to get the diff from a Git branch hosted on GitHub and create a branch from that.


I see that the new str2response method is only used inside the twisted/web/error.py module. Do we need it in twisted/web/_responses.py ? Maybe just make it a private fuction in twisted/web/error.py and skip the test for it.. but do keep the tests for the public errors using this code.

All tests need docstring... including the new one for str2response but I hope we can make that code private.

Also you will need to follow the Twisted coding convention, sorry. 2 new lines between methods ... 3 new lines between classed. I know that this is not easy.

Once patch applies on trunk I can send the patch to the twistedchecker builder and we can see all errors.

Thanks!

comment:3 Changed 3 years ago by Dustin J. Mitchell

Here's the patch -- https://github.com/djmitche/Twisted/commit/8e537a20774e40d5e321a4afd5043c0295fb4f52. I think it doesn't apply since your and hawkowl's py3 patches have landed since I wrote it.

I put str2response in _responses.py since it's about responses and is essentially the inverse of RESPONSES. I'd ratehr keep it there, but I can add the unit test docstring. I'll hold off on that until you've had the rest of a look.

comment:4 Changed 3 years ago by Adi Roiban

Keywords: review added

Thanks. Let's put this ticket back in the review queue.

comment:5 Changed 3 years ago by Adi Roiban

Author: adiroibandustin

comment:6 Changed 3 years ago by Adi Roiban

Owner: Dustin J. Mitchell deleted

comment:7 Changed 3 years ago by Adi Roiban

Author: dustinadiroiban, dustin
Branch: branches/twisted.web.error-py3-7845branches/twisted.web.error-py3-7845-2

(In [44569]) Branching to twisted.web.error-py3-7845-2.

comment:8 Changed 3 years ago by Adi Roiban

Author: adiroiban, dustindustin
Branch: branches/twisted.web.error-py3-7845-2branches/twisted.web.error-py3-7845
Keywords: review removed
Owner: set to Dustin J. Mitchell

Thanks for the git commit... Much easier to work with.

I solved the conflicts but please check that the merge is valid in the new branch.


I don't want to force you to move str2response ... but I think that it should be renamed codeToPhrase or something to hint about rfc2616 - 6.1.1 Status Code and Reason Phrase and to not use leetspeak.

I am not sure what is Twisted policy regarding leetspeak in method names... so this is only a personal request.

Also, in case you want to have str2response in _response.py please update the module docstring: HTTP response code definitions. to something like HTTP response code definitions and helpers to handle status code and reason phrases.


Changes looks good otherwise.

Many thanks for the new tests!

I think that instead of assertIdentical we can use assertIs as defined in stdlib unit test.

Please update docstrings for str2response testes and for the other new methods.

Docstring for new test methods are great. Thanks!

Also for class docstrings for RedirectWithNoLocationTests ... I am not sure if the class docstring should contain ' ... are initialized and displayed.' as this info should go into the test method docstring... and later we can extend RedirectWithNoLocationTests with tests which does not check for initialization and display.

Also, I think that "Tests for how" part can be left out as in the context of test code it is redundant.

In the case of RedirectWithNoLocationTests ... I think that any docstring is redundant :( ... I will open a discussion in twisted mail list and see what can be done.

For things like self.assertEqual(e.message, "NO LOCATION to https://example.com") can you please add explicit markup for text or bytes ?


I have sent the branch to buildbot and waiting for results.

I think that the branch is is good shape and it only need a bit of cleanup.

Please check the above comments and buildbot / twistedchecker results.

In case you don't have time to do the change please let us know and I will try to continue working on this.

Many thanks again!

comment:9 Changed 3 years ago by Adi Roiban

The following tests are failing on buildbots and only on my local system ... they fain on py2.7 and py2.6

[FAIL]
Traceback (most recent call last):
  File "/home/buildslave/run/debian6-x86_64-py2.6-glib/Twisted/twisted/web/test/test_flatten.py", line 448, in test_string
    "Exception while flattening:\n"
  File "/home/buildslave/run/debian6-x86_64-py2.6-glib/Twisted/twisted/trial/_synctest.py", line 437, in assertEqual
    super(_Assertions, self).assertEqual(first, second, msg)
  File "/usr/lib/python2.6/unittest.py", line 350, in failUnlessEqual
    (msg or '%r != %r' % (first, second))
twisted.trial.unittest.FailTest: "Exception while flattening:\n  '01234567890123456789<...>1234567890123456789'\nRuntimeError: reason\n" != "Exception while flattening:\n  '01234567890123456789<...>01234567890123456789'\nRuntimeError: reason\n"

twisted.web.test.test_flatten.FlattenerErrorTests.test_string
===============================================================================
[FAIL]
Traceback (most recent call last):
  File "/home/buildslave/run/debian6-x86_64-py2.6-glib/Twisted/twisted/web/test/test_flatten.py", line 469, in test_unicode
    "Exception while flattening:\n"
  File "/home/buildslave/run/debian6-x86_64-py2.6-glib/Twisted/twisted/trial/_synctest.py", line 437, in assertEqual
    super(_Assertions, self).assertEqual(first, second, msg)
  File "/usr/lib/python2.6/unittest.py", line 350, in failUnlessEqual
    (msg or '%r != %r' % (first, second))
twisted.trial.unittest.FailTest: "Exception while flattening:\n  u'01234567\\u2603901234567\\u2603901234567\\u2603901234567\\u2603901234567\\u2603901234567\\u2603901234567\\u2603901234567\\u2603901234567\\u2603901234567\\u26039'\nRuntimeError: reason\n" != "Exception while flattening:\n  u'01234567\\u2603901234567\\u26039<...>01234567\\u2603901234567\\u26039'\nRuntimeError: reason\n"

twisted.web.test.test_flatten.FlattenerErrorTests.test_unicode

comment:10 Changed 3 years ago by Dustin J. Mitchell

I don't much care for the trivialities of naming or docstrings. As you'd prefer I'll call it codeToPhrase and put it in error.py as a private function. The docstrings for the new tests classes are copied from the first test in the file, and I agree that there's not much to say there -- the methods barely have behavior, not to speak of the classes. But RedirectWithNoLocationTests's docstring is definitely wrong.

I'll work on the test errors.

comment:11 Changed 3 years ago by Adi Roiban

In case my comments don't make sense you should not follow them.

Thanks for the feedback!

comment:12 Changed 3 years ago by Dustin J. Mitchell

Keywords: review added
Owner: Dustin J. Mitchell deleted

They make sense - I'm just not going to argue :)

Oh, and as for the encoding of the message - it's always a native string: bytes (str) on python 2 and unicode (str) on python 3.

I've pushed a new version to the branch. Aside from what was requested, I moved two of the FlattenerError tests out of _flatten.py since they were testing the class's behavior, not the flattener. I adjusted how that error is rendered, too, to match the existing behavior regarding exactly where the ellipsis is inserted. I also adjusted the test to expect a ..u'.. within the exception on Python 2, and not on Python 3. This now passes both under Python 2 and 3 for me.

comment:13 Changed 3 years ago by Adi Roiban

I don't know what happened with the branch attached to this ticket...

The new branch should be https://github.com/twisted/twisted/compare/trunk...twisted.web.error-py3-7845-2

I could not find the branch you were referring the previous comment.

Can you please add an explicit link to it?

Thanks!

comment:15 Changed 3 years ago by Glyph

Author: dustinglyph, dustin
Branch: branches/twisted.web.error-py3-7845branches/py3-twisted-web-error-7845

(In [44706]) Branching to py3-twisted-web-error-7845.

comment:16 Changed 3 years ago by Glyph

I put the branch into SVN to give the builders a crack at it; initial results are promising; one pyflakes issue.

comment:17 Changed 3 years ago by Glyph

There's already a separate ticket, #7811, for porting twisted.web.template, and that work should probably be completed separately. Sorry for the poor record of dependencies :-\. (This comment doesn't constitute a full review, so I'm not removing the tag.)

comment:18 Changed 3 years ago by Dustin J. Mitchell

This doesn't port twisted.web.template - just moves it to the "almost" set since it's imported (at runtime) by twisted.web.error

comment:19 Changed 3 years ago by Glyph

Oh. Well, given that #7811 doesn't actually have any code on it perhaps we should just proceed then :).

comment:20 Changed 3 years ago by Dustin J. Mitchell

Proceed with the review? I neither ported nor intend to port twisted.web.template, so I'm not sure how #7811 is relevant at all.

comment:21 in reply to:  20 Changed 3 years ago by Glyph

Replying to dustin:

Proceed with the review? I neither ported nor intend to port twisted.web.template, so I'm not sure how #7811 is relevant at all.

It's not relevant. What I was saying was, please disregard my earlier comment, since there are no conflicts to worry about or inverted dependencies, this should be reviewed (and hopefully landed) as-is.

comment:22 Changed 3 years ago by hawkowl

(In [44821]) add error as a module and test_error as the tests (refs #7845)

comment:23 Changed 3 years ago by hawkowl

(In [44822]) add urlparse to all (refs #7845)

comment:24 Changed 3 years ago by hawkowl

Keywords: review removed

Rather than send it through another cycle, I just went through and fixed some minor things (pyflakes, twistedchecker that the bots picked up on, some {{{future}} module imports).

As a reviewer, I am happy with this. But as I *have* made some changes, I'm putting this up for re-review. Builders spun.

Thanks for the patch, dustin :)

comment:25 Changed 3 years ago by hawkowl

Keywords: review added

As I said, putting it back up for review.

comment:26 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

Changes look good to merge.

Do we need CodeToMessageTests ? I was expecting to keep it private and only have test for the public code using it.

In this way we can refactor the code (including a version which does not use this method) without having to care about CodeToMessageTests

Hawkie, I am putting this on your back to review my comment and merge :)

Thank you all!

comment:27 Changed 3 years ago by hawkowl

Hi adi,

Direct unit tests are always better than having it tested by non-direct integration tests -- if we refactor it, we'll just remove the private function and the tests.

Will merge.

comment:28 Changed 3 years ago by hawkowl

Author: glyph, dustinhawkowl, dustin, glyph
Branch: branches/py3-twisted-web-error-7845branches/py3-twisted-web-error-7845-2

(In [44907]) Branching to py3-twisted-web-error-7845-2.

comment:29 Changed 3 years ago by hawkowl

Resolution: fixed
Status: newclosed

(In [44911]) Merge py3-twisted-web-error-7845-2: Port twisted.web.error to Python 3

Author: dustin Reviewers: adiroiban, hawkowl Fixes: #7845

Note: See TracTickets for help on using tickets.