Opened 3 years ago

Closed 3 years ago

#7795 enhancement closed fixed (fixed)

twisted.web.xmlrpc is not ported to Python 3

Reported by: Einar Fløystad Dørum Owned by: hawkowl
Priority: normal Milestone: Python-3.x
Component: web Keywords:
Cc: Branch: branches/xmlrpc-py3-7795-6
branch-diff, diff-cov, branch-cov, buildbot
Author: hawkowl, adiroiban, glyph

Description

The twisted.web.xmlrpc module has not been ported to Python3

Attachments (10)

xmlrpc_py3.patch (33.0 KB) - added by Einar Fløystad Dørum 3 years ago.
Patch to add python 3 support to xmlrpc
xmlrpc_py3-2.patch (34.7 KB) - added by Einar Fløystad Dørum 3 years ago.
Version 2 of the fix
xmlrpc_py3-3.patch (20.6 KB) - added by Einar Fløystad Dørum 3 years ago.
Patch without twisted.web.static.py dependency
xmlrpc_py3-4.patch (22.1 KB) - added by otonsber 3 years ago.
v4, with pyflakes and other review cleanups
xmlrpc_py3-5.patch (25.2 KB) - added by Einar Fløystad Dørum 3 years ago.
xmlrpc_py3-6.patch​ (25.4 KB) - added by Einar Fløystad Dørum 3 years ago.
xmlrpc_py3-7.patch​​ (25.6 KB) - added by Einar Fløystad Dørum 3 years ago.
xmlrpc_py3-handler-7.patch​​ (4.1 KB) - added by Einar Fløystad Dørum 3 years ago.
Fix for handler issue
xmlrpc_py3-8.patch (25.1 KB) - added by Einar Fløystad Dørum 3 years ago.
xmlrpc_py3-9.patch (25.2 KB) - added by Einar Fløystad Dørum 3 years ago.

Download all attachments as: .zip

Change History (59)

Changed 3 years ago by Einar Fløystad Dørum

Attachment: xmlrpc_py3.patch added

Patch to add python 3 support to xmlrpc

comment:1 Changed 3 years ago by Einar Fløystad Dørum

Keywords: review added

comment:2 Changed 3 years ago by Adi Roiban

Author: adiroiban
Branch: branches/xmlrpc-py3-7795

(In [43939]) Branching to xmlrpc-py3-7795.

comment:3 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: set to Einar Fløystad Dørum

Hi,

Thanks for working on this.

Please note that I am still a junior reviewer so at this stage I am just doing a first level / quick review. In case some of my comments don't make sense, please ignore them and just resubmit the ticket for reivew and wait for another reviewer to check the code.

The final patch will need a news files as described here: http://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles


In twisted/persisted/styles.py why not use the following code to replace the conditional import?

from twisted.python.compat import NativeStringIO

In twisted/web/test/test_xmlrpc.py , twisted/python/urlpath.py and twisted/web/xmlrpc.py maybe instead of having all these conditional _PY3 import for stdlib reorganization we should have them defined in twisted.python.compat . This should reduce duplication in all these conditional imports.

---

In twisted/web/xmlrpc.py

            request.setHeader(b"content-length", intToBytes(len(content))) 
            request.write(content.encode('utf8'))

I think that this is wrong.

First this method should already received the content encoded in some way and don't assume that everybody wants to use UTF-8 to serialize Unicode. Second, the length of UTF-8 serialization is different than source Unicode. In the HTTP headers you should advertise the length of the encoded data, not of the Unicode data.

Here is an example

>>> ca = u'prț'
>>> len(ca)
3
>>> len(ca.encode('utf-8'))
4

Same story for

        self.transport.write(self.factory.payload.encode('utf8')) 

I have also sent the patch to buildbot builders. Will wait for results.

Thanks!

comment:4 in reply to:  3 ; Changed 3 years ago by Einar Fløystad Dørum

Replying to adiroiban:

Hi,

Thanks for working on this.

Please note that I am still a junior reviewer so at this stage I am just doing a first level / quick review. In case some of my comments don't make sense, please ignore them and just resubmit the ticket for reivew and wait for another reviewer to check the code.

The final patch will need a news files as described here: http://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles


I missed that, will fix.

In twisted/persisted/styles.py why not use the following code to replace the conditional import?

from twisted.python.compat import NativeStringIO

Later in the file, there is code that does conditional handling if the StringIO has an OutputType or InputType. Those only exists for cStringIO, and never for NativeStringIO, so that is the reason for not using NativeStringIO there.

But if the OutputType / InputType logic is not important. Then removing that and using NativeStringIO would be nicer.

In twisted/web/test/test_xmlrpc.py , twisted/python/urlpath.py and twisted/web/xmlrpc.py maybe instead of having all these conditional _PY3 import for stdlib reorganization we should have them defined in twisted.python.compat . This should reduce duplication in all these conditional imports.

---

I will look into that.

In twisted/web/xmlrpc.py

            request.setHeader(b"content-length", intToBytes(len(content))) 
            request.write(content.encode('utf8'))

I think that this is wrong.

First this method should already received the content encoded in some way and don't assume that everybody wants to use UTF-8 to serialize Unicode.

Content-type is text/xml, which implies that the encoding is UTF-8. Adding support for selecting encoding to xmlrpc might be of some value. But isn't it a separate concern from porting to Python 3?

Second, the length of UTF-8 serialization is different than source Unicode. In the HTTP headers you should advertise the length of the encoded data, not of the Unicode data.

Here is an example

>>> ca = u'prț'
>>> len(ca)
3
>>> len(ca.encode('utf-8'))
4

Good catch, will create a new patch with this issue fixed.

comment:5 in reply to:  4 Changed 3 years ago by Einar Fløystad Dørum

Replying to edorum:

Replying to adiroiban:

In twisted/web/test/test_xmlrpc.py , twisted/python/urlpath.py and twisted/web/xmlrpc.py maybe instead of having all these conditional _PY3 import for stdlib reorganization we should have them defined in twisted.python.compat . This should reduce duplication in all these conditional imports.

---

I will look into that.

The xmlrpc.client library is only for xmlrpc and will not be used for anything other than a xmlrpc client. Does it really make sense to have that in compat which everything will be using?

Changed 3 years ago by Einar Fløystad Dørum

Attachment: xmlrpc_py3-2.patch added

Version 2 of the fix

comment:6 Changed 3 years ago by Adi Roiban

Keywords: review added
Owner: Einar Fløystad Dørum deleted

In case xmlrpc.client is only used in a single part it should not to be in compat.

I will not have time to review it in the next days so let's put it for general review.

Thanks!

comment:7 Changed 3 years ago by Adi Roiban

I have applied the new patch on top of current branch and send it to buildbot... there is still no news file.

Cheers

comment:8 in reply to:  7 Changed 3 years ago by Einar Fløystad Dørum

Replying to adiroiban:

I have applied the new patch on top of current branch and send it to buildbot... there is still no news file.

I added twisted/web/topfiles/7795.feature , shouldn't it cover that?

comment:9 Changed 3 years ago by Adi Roiban

Sorry. Should be fine... I forget that patch is ignoring those files. Thanks!

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

Keywords: review removed
Owner: set to Einar Fløystad Dørum

Sorry. Should be fine... I forget that patch is ignoring those files. Thanks!

Patch doesn't ignore news files. It ignores empty files. .feature files aren't supposed (or allowed) to be empty. And the one in this patch file is not.

More likely you didn't manually svn add the file, I guess.

Separately, looking through the patch file, it seems that this change touches far more than twisted.web.xmlrpc but it doesn't touch any test files except test_xmlrpc. This suggests there is some other porting work to do *before* this work. If there are unported dependencies of twisted.web.xmlrpc, they and their test suites must be ported first.

Thanks.

Changed 3 years ago by Einar Fløystad Dørum

Attachment: xmlrpc_py3-3.patch added

Patch without twisted.web.static.py dependency

comment:11 Changed 3 years ago by Einar Fløystad Dørum

The third patch stopped using twisted.web.static, which removed all but one of the problematic dependencies. The only one left is twisted.web.server, but that one is already in use by other Python 3 ported code without tests.

comment:12 Changed 3 years ago by Einar Fløystad Dørum

Keywords: review added
Owner: Einar Fløystad Dørum deleted

comment:13 Changed 3 years ago by Glyph

Author: adiroibanadiroiban, glyph
Branch: branches/xmlrpc-py3-7795branches/xmlrpc-py3-7795-2

(In [43945]) Branching to xmlrpc-py3-7795-2.

comment:14 Changed 3 years ago by Glyph

Patch applied to a branch, builds run. Enjoy, future reviewers :).

comment:15 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: set to Einar Fløystad Dørum

thanks for the changes.

errors from pyflakes

twisted/web/test/test_xmlrpc.py:514: redefinition of unused 'resource' from line 23
twisted/web/test/test_xmlrpc.py:709: undefined name 'intToBytes'

errors from twistedchecker

************* Module twisted.web.server
W9208:  1,0: Missing docstring
************* Module twisted.web.test.test_xmlrpc
W9208:  1,0: Missing docstring
C0301:481,0: Line too long (83/79)
C0301:827,0: Line too long (82/79)
************* Module twisted.web.xmlrpc
W9208:  1,0: Missing docstring
C0301:192,0: Line too long (81/79)
C0301:511,0: Line too long (82/79)

btw. pyflakes is now pretty clean and you can run pyflakes twisted/web to check that your changes are accepted by pyflakes


In twisted/web/server.py from __future__ import division, absolute_import is duplicated.

in twisted/web/test/test_xmlrpc.py and twisted/web/xmlrpc.py I think that from future import should come after module's docstring. I think that a module structure should be:

  1. Copyright header
  2. Module general docstring
  3. stdlib imports
  4. 3rd party imports
  5. project imports.

Maybe StaticData should be renamed to ResourceWithInvalidResponse ... this should avoid the need to document the class name with generic StaticData.

Also in StaticData definition, maybe isLeaf should be defined before render_GET, in this way all class members are located in a common place.


not sure if this is a good idea but in twisted/web/test/test_xmlrpc.py instead of replication the logic for importing the xmlrpclib maybe you can import it direclty from twisted.web.xmlrpc like from twisted.web.xmlrpc import xmlrpclib

but maybe is ok... is just that I have an obsession with removing duplicate code.


Beside fixing the content-length issue, we should also write a test to check for that. Right now all tests are based on ascii text so if instead of

            request.setHeader(b"content-length", intToBytes(len(encodedContent)))

I use

            request.setHeader(b"content-length", intToBytes(len(content)))

this suite will still pass... instead of failing

trial twisted.web.test.test_xmlrpc

Please update current tests to use unicode data or create a new tests for this case.

All changes needs tests.


Now, I see that python3 tests pass but they produce many Unhandled Error which are ignored by current test runner. Hope that soon, we can have Trial ported.

Thanks!

Changed 3 years ago by otonsber

Attachment: xmlrpc_py3-4.patch added

v4, with pyflakes and other review cleanups

comment:16 Changed 3 years ago by otonsber

Keywords: review added
Owner: Einar Fløystad Dørum deleted

edorum has some login issues, posting updated patch on his behalf.

comment:17 Changed 3 years ago by Glyph

otonsber, thanks for doing that.

We had a couple of authentication database issues which may have prevented edorum from logging in. If you can have them get in touch over email I can manually reset their password.

comment:18 Changed 3 years ago by Adi Roiban

Branch: branches/xmlrpc-py3-7795-2branches/xmlrpc-py3-7795-3

(In [44042]) Branching to xmlrpc-py3-7795-3.

comment:19 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: set to Einar Fløystad Dørum

Thanks for the patch. I have sent it to buildbots.

All new methods, functions, classed needs great docstrings, including those from tests both the test method itsfelf the testcase itself but also any other helper function/class/method.


Instead of

from twisted.web import resource as web_resource

why not do

from twisted.web.resource import Resource

I am not a big fan of module renaming at import


This patch also need a new test for checking that content-length is correctly set for Unicode data.


I am not happy with this code in xmlrpc.py

        if _PY3:
            payload = self.factory.payload.encode('utf8')
        else:
            payload = self.factory.payload

I would prefer to always convert unicode text to UTF-8 encoding, even when pass to Python 2. In this way we don't need python3 to run a test for this code.

In case you need different behaviour for py2 and py3 please encapsulate it into a helper function and place it in the compat module together with tests for it.

Each "if" inside a code requires at least 2 tests. Your patch introduces many IFs without any new tests.

The current code might work for your usecase, but without comprehensive tests we can not be sure if the changes does not break other usecases.

Also, writing tests for the changes will ensure that future changes (probably done by another person) will not break your usecase.


I see that a lot of calls to xmlrpc.Proxy are done by wrapping the input string as nativeString. Why not do this conversion inside the init method of xmlrpc.Proxy ?

This should be in sync with the transparent conversion to UTF-8 which is done for request's content.

In this way we can use Unicode for all xmlprc.Proxy calls. It should be a much nicer API.

If this require a lot of changes, it can be left for another ticket... but since Pyhon3 support a nice way of working with Unicode it would be a shame to dumb down the xmlrpc.Proxy API.


Please review my comments and either submit a new patch or add a (short) comment why you think my requirements are bad.

Thanks!

comment:20 Changed 3 years ago by Adi Roiban

Buildbot are almost green, but this is because we lack a builder to complain on missing code coverage.


Please note the twistedchecker errors

(view as text)
************* Module twisted.web.test.test_xmlrpc
W9208:144,4:Test.xmlrpc_snowman: Missing docstring
W9015:722,0: Too many blank lines, found 2

Another comment regarding the

if _PY3:
   payload = self.factory.payload.encode('utf8')
else:
   payload = self.factory.payload

we need to update the docstring of the class and inform that on Python3 xmlrpc_* methods must return Unicode... just returning binary data / already encoded result will fail. And the docstring also needs to be updated to inform that on Python2 the xmprc_* methods must return binary data.

This is why I think that by changing the code to handle Unicode/binary data independent of Python version the docstring will be simpler and API much nicer.

Thanks!

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

Thanks.

The third patch stopped using twisted.web.static, which removed all but one of the problematic dependencies. The only one left is twisted.web.server, but that one is already in use by other Python 3 ported code without tests.

This is incorrect. There are tests for twisted.web.server in twisted.web.test.test_web and twisted.web.test.test_web is included in testModules in twisted.python.dist3.

Even if there were no tests for twisted.web.server on Python 3, that would just indicate that someone previously made a mistake - it would not be a justification to continue to do things wrong.

Please add tests to twisted.web.test.test_web covering the changes you've made to twisted.web.server on Python 3.

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

Buildbot are almost green, but this is because we lack a builder to complain on missing code coverage.

adiroiban, is this an oblique request for the submitter to add the missing test coverage? If so, please be a bit more direct in the future. If not ... what are you saying there?

comment:23 Changed 3 years ago by Adi Roiban

I was saying that even though the buildbot builders are green the patch is not ready to land.

In comments 15 and 19 I have already asked for tests to cover current changes.

comment:24 in reply to:  19 Changed 3 years ago by Einar Fløystad Dørum

Replying to adiroiban:

Since I can't login with my old user anymore, and trac's reset_password functionality didn't work. I've now changed name.

This patch also need a new test for checking that content-length is correctly set for Unicode data.

If the content-length is wrong, the unicode test will fail, isn't that good enough? This is also the reason why xmlrpc_snowman takes an argument.

I am not happy with this code in xmlrpc.py

        if _PY3:
            payload = self.factory.payload.encode('utf8')
        else:
            payload = self.factory.payload

I would prefer to always convert unicode text to UTF-8 encoding, even when pass to Python 2. In this way we don't need python3 to run a test for this code.

In case you need different behaviour for py2 and py3 please encapsulate it into a helper function and place it in the compat module together with tests for it.

Each "if" inside a code requires at least 2 tests. Your patch introduces many IFs without any new tests.

Without the if's it either breaks on Python 3 or on Python 2. The reason for this is that in Python 3 the return values from the xmlrpclibs.dumps and loads have changed from a binary string to unicode. But it might be better to move the if's closer to those two functions. Maybe create compat version of them. But that would be a xmlrpc specific compat. So should I do that, and should I put those in compat.py, or keep them in xmlrpc.py?

comment:25 Changed 3 years ago by Adi Roiban

In case you patch is good to merge or for review please add the 'review' tag. Otherwise there is the risk of loosing it among so many comments.


yes, with the new xmlrpc_snowman method the content-length is tested using test_results but I think that we need a dedicate test method for content-length with a dedicated docstring.


there is this error

test_doubleEncodingError ... Unhandled Error
Traceback (most recent call last):
  File "/home/adi/chevah/twisted/twisted/web/xmlrpc.py", line 170, in render_POST
    d.addCallback(self._cbRender, request, responseFailed)
  File "/home/adi/chevah/twisted/twisted/internet/defer.py", line 307, in addCallback
    callbackKeywords=kw)
  File "/home/adi/chevah/twisted/twisted/internet/defer.py", line 296, in addCallbacks
    self._runCallbacks()
  File "/home/adi/chevah/twisted/twisted/internet/defer.py", line 578, in _runCallbacks
    current.result = callback(current.result, *args, **kw)
--- <exception caught here> ---
  File "/home/adi/chevah/twisted/twisted/web/xmlrpc.py", line 190, in _cbRender
    allow_none=self.allowNone)
  File "/home/adi/chevah/twisted/twisted/web/test/test_xmlrpc.py", line 405, in fakeDumps
    raise RuntimeError("Cannot encode anything at all!")
builtins.RuntimeError: Cannot encode anything at all!
ok


test_errors ... Unhandled Error
Traceback (most recent call last):
  File "/home/adi/chevah/twisted/twisted/web/server.py", line 189, in process
    self.render(resrc)
  File "/home/adi/chevah/twisted/twisted/web/server.py", line 238, in render
    body = resrc.render(self)
  File "/home/adi/chevah/twisted/twisted/web/resource.py", line 250, in render
    return m(request)
  File "/home/adi/chevah/twisted/twisted/web/xmlrpc.py", line 168, in render_POST
    d = defer.maybeDeferred(function, *args)
--- <exception caught here> ---
  File "/home/adi/chevah/twisted/twisted/internet/defer.py", line 140, in maybeDeferred
    result = f(*args, **kw)
  File "/home/adi/chevah/twisted/twisted/web/test/test_xmlrpc.py", line 136, in xmlrpc_fail
    raise TestRuntimeError
twisted.web.test.test_xmlrpc.TestRuntimeError: 
Unhandled Error
Traceback (most recent call last):
Failure: twisted.web.test.test_xmlrpc.TestValueError: 
ok


we need to investigate why we get these unhandled errors on py3.


Regarding

        if _PY3:
            payload = self.factory.payload.encode('utf8')

The if is important and should exists. Either here or upstream. My problem is with the _PY3 part and the fact that you need python2 AND python3 to check the code.

from twisted.python.compat import unicode

if isinstance(self.factory.payload, unicode):
    payload = self.factory.payload.encode('utf8')
else:
    payload = self.factory.payload


if isinstance(content, unicode)::
    content = content.encode('utf8')

So instead of the generic PY3 check we should check for the specific behaviour. With my changes, even if at some time in the future Py2 API start to return Unicode, the code will still works and you can also unit test the py3 behaviour on python2

but this is just a minor comment.


To summarize.

Buildbots need to be green to merge this branch. This mean fixing the docstring.

Unhandled errors are bad and we should look at them and if we can not fix them here, create a follow-up ticket. I have started a discussion on the mailing list to check that status of these Unhandled Errors in python3

Thanks for the work on this!

Changed 3 years ago by Einar Fløystad Dørum

Attachment: xmlrpc_py3-5.patch added

comment:26 Changed 3 years ago by Einar Fløystad Dørum

Keywords: review added

Adding a new patch, I believe that it addresses the issues raised in earlier comments. It does not change the URL type from bytes, I had a look at other places that takes an URL and it looked to me like they usually where bytes. So it seemed reasonable to use bytes here as well.

comment:27 Changed 3 years ago by Chris Wolfe

Owner: Einar Fløystad Dørum deleted

Hi edorum2 - Thanks for your work!

Just an FYI - when submitting a ticket for review, please reassign to the blank user. This will make it obvious to others that anyone can review the ticket.

https://twistedmatrix.com/trac/wiki/ReviewProcess#Authors:Howtogetyourchangereviewed

Thanks again!

comment:28 Changed 3 years ago by Einar Fløystad Dørum

Did I forget something that is needed to get the last patch reviewed?

comment:29 Changed 3 years ago by Glyph

Sorry edorum2! Sometimes patches just sit for a little while, you've done everything you need to. Reminders never hurt, though :-). Particularly, hawkowl is working actively on python 3 porting right now, so if you can ping her on #twisted-dev on IRC she might show up and review this.

comment:30 Changed 3 years ago by adiroiban1

Just an info comment... not a full review

The last patch still has missing docstrings.

Docstrings are required and documented as part of Twisted coding standard http://twistedmatrix.com/documents/current/core/development/policy/coding-standard.html

The new code should explain that the purpose of snowman method is to check the handling of Unicode text

in twisted/web/test/test_web.py the new code need to go in a dedicated test method with a dedicated docstring

I am leaving this in the review queue so that another reviewer can pick it up.


Somehow I failed to stress the importance of well documented code and the requirement of the docstrings and I silently gave up reviewing this patch :(

We really should have a better twistedchecker tool which can be run on local computers together with pyflakes so that reviewers don't have to waste time complaining about missing docstrings.

Good luck!

Changed 3 years ago by Einar Fløystad Dørum

Attachment: xmlrpc_py3-6.patch​ added

comment:31 Changed 3 years ago by Einar Fløystad Dørum

I've added a patch that adds docstring to all new functions, not just substantial ones, and splits out the 405 testing in test_web.py in its own function.

I believe that this should address adiroiban1 reservations.

comment:32 Changed 3 years ago by hawkowl

Owner: set to hawkowl

Hi edorum2, thanks for your work.

I'll do a full review of this ticket tonight. :)

comment:33 Changed 3 years ago by hawkowl

Author: adiroiban, glyphhawkowl, adiroiban, glyph
Branch: branches/xmlrpc-py3-7795-3branches/xmlrpc-py3-7795-4

(In [44693]) Branching to xmlrpc-py3-7795-4.

comment:34 Changed 3 years ago by hawkowl

(In [44694]) apply patch from edorum2, refs #7795

comment:35 Changed 3 years ago by hawkowl

Keywords: review removed
Owner: changed from hawkowl to Einar Fløystad Dørum

Hi edorum2,

Many thanks for the work. I'm happy to see more things get ported. :) Here's my review comments:

  • There's some minor conflicts from trunk. I fixed them up, but you might want to merge forward your local changes.
  • Coverage looks good, except one section of xmlrpc isn't tested (https://buildbot.twistedmatrix.com/builds/twisted-coverage.py/twisted-py3-coverage.py-r1fdaeec90f27095b086961f7c37738b1d45038d5/twisted_web_xmlrpc.html) -- would you be able to write a test for that?
  • twisted/web/server.py: At line 269, the formatting won't do what you want it to do on Python 3. It will instead return "our browser approached me (at /) with the method "b'GET'". I only allow the method b'POST' here." which is not what we want. You will need to wrap method and allowed in nativeString. Could you also amend the test to make sure that "b'" is not in anywhere in the response, as this indicates the repr of a bytestring.
  • twisted/web/test/test_web.py: You removed a newline at line 764. As Twisted has 3 lines between top level things, this should go back.
  • twisted/web/test/test_xmlrpc.py: The Twisted coding style of doing multiple imports is doing them without () - so multiple {{from twisted.web.xmlrpc}}}s, please.
  • twisted/web/test/test_xmlrpc.py: Twisted docstrings are in the format of
    """
    Comment.
    """
    

even for one-line docstrings. Please amend them. There's also no newline after the docstring (line 331) and two lines between second-level functions (line 353).

  • twisted/web/xmlrpc.py: I think that instead of text/xml we should instead have text/xml; charset=utf-8. Especially since there's a section where implicit decoding is done.
  • twisted/web/xmlrpc.py: Could you please amend all cases of C{bytes} to be L{bytes}? This makes the documentation better by linking to the bytes object in the Python documentation.

Otherwise, I think it's in good shape. :)

Again, thanks for this, and I'm glad to see more Python 3 work going on. Please fix the issues I have raised above and resubmit.

Last edited 3 years ago by hawkowl (previous) (diff)

Changed 3 years ago by Einar Fløystad Dørum

Attachment: xmlrpc_py3-7.patch​​ added

Changed 3 years ago by Einar Fløystad Dørum

Fix for handler issue

comment:36 Changed 3 years ago by Einar Fløystad Dørum

Keywords: review added
Owner: Einar Fløystad Dørum deleted

I've added a new patch (xmlrpc_py3-7.patch), that should addresses all but the coverage reservation from hawkowl.

When adding coverage I found the following. Inside the _cbRender function in the XMLRPC class, the result field of the Handler class gets passed to xmlrpclib.dumps, it is a deferred, and that dumps function is not part of twisted, and does not support deferred. So as far as I can see this is a bug. But this bug doesn't have anything to with the porting changes, so I split the fix for it out in a separate patch (xmlrpc_py3-handler-7.patch​​​). So you can take or not take, that fix separately from the porting work.

comment:37 Changed 3 years ago by hawkowl

Owner: set to hawkowl

Hi edorum2, thanks for the patch. I'll run it on the buildbots tomorrow and review it.

Could you please open a ticket for the bug with the details you've found? Since it's a bug, and coverage can't reasonably be added for the buggy code, I think that it's fine to have that bit of coverage red, and to have it be covered in the bugfix with a (py3 compatible) test case for it in that ticket, so that this one can be merged sooner.

Thanks again for your work!

comment:38 Changed 3 years ago by Einar Fløystad Dørum

Added https://twistedmatrix.com/trac/ticket/7896 to track the Handler issue.

comment:39 Changed 3 years ago by hawkowl

Branch: branches/xmlrpc-py3-7795-4branches/xmlrpc-py3-7795-5

(In [44817]) Branching to xmlrpc-py3-7795-5.

comment:40 Changed 3 years ago by hawkowl

(In [44818]) apply patch from edorum2, refs #7795

comment:41 Changed 3 years ago by hawkowl

Component: coreweb
Keywords: review removed
Owner: changed from hawkowl to Einar Fløystad Dørum

Thanks for the patch and your continued efforts :)

Two last things:

  1. https://buildbot.twistedmatrix.com/builds/twisted-coverage.py/twisted-py3-coverage.py-re110359f1bfff8177455910014ca064e7a2af9b2/twisted_web_test_test_xmlrpc.html#n747 has no coverage, could you please revisit this? Maybe assert that the content of the exception is the error, rather than just a general exception.
  2. The topfile should be "twisted.web.xmlrpc is now ported to Python 3."

Please fix these two issues and resubmit. I think that should be all from me after that. Thanks again!

Changed 3 years ago by Einar Fløystad Dørum

Attachment: xmlrpc_py3-8.patch added

Changed 3 years ago by Einar Fløystad Dørum

Attachment: xmlrpc_py3-9.patch added

comment:42 Changed 3 years ago by Einar Fløystad Dørum

Keywords: reviews added
Owner: Einar Fløystad Dørum deleted

Added patch xmlrpc_py3-9.patch that should address those two issues.

Last edited 3 years ago by Einar Fløystad Dørum (previous) (diff)

comment:43 Changed 3 years ago by Einar Fløystad Dørum

Keywords: review added; reviews removed

comment:44 Changed 3 years ago by hawkowl

Branch: branches/xmlrpc-py3-7795-5branches/xmlrpc-py3-7795-6

(In [44827]) Branching to xmlrpc-py3-7795-6.

comment:45 Changed 3 years ago by hawkowl

(In [44828]) apply patch from edorum2, refs #7795

comment:46 Changed 3 years ago by hawkowl

Keywords: review removed
Owner: set to hawkowl

Thanks edorum2, the patch looks good. I've fixed up some little things that aren't worth another review cycle -- I'll double check when I'm more awake tomorrow, and merge it if there's nothing else to do with it. :)

Thanks again for your hard work!

comment:47 Changed 3 years ago by Glyph

Hi hawkowl,

For future reference, the way that "minor changes" like this are supposed to work are:

  1. write out the review for the original person
  2. determine if, assuming if the author were a committer, you would say "merge after fixing…" or "fix and resubmit…"
  3. if it's the latter, post the review as normal; if the former, say "I will address the minor feedback"

This way the author gets to see the full review but doesn't have to wait for another cycle, since they wouldn't have to wait if they were a committer.

comment:48 Changed 3 years ago by hawkowl

Thanks Glyph, noted for later :)

As far as the items that I'm fixing:

  • There's some new pyflakes errors (intToBytes and Resource is no longer used)
  • twisted/web/test/test_web.py:735 - stray newline
  • twisted/web/test/test_xmlrpc.py - missing full stops in some new docstrings.

I'm going to address the last two (I did the pyflakes error when I saw it last night) and merge. :)

comment:49 Changed 3 years ago by hawkowl

Resolution: fixed
Status: newclosed

(In [44831]) Merge xmlrpc-py3-7795-6: Port twisted.web.xmlrpc to Python 3

Author: edorum Reviewers: adiroiban, exarkun, hawkowl Fixes: #7795

Note: See TracTickets for help on using tickets.