Opened 6 years ago

Closed 4 years ago

#3219 enhancement closed fixed (fixed)

Support for use_datetime in twisted.web.xmlrpc

Reported by: chadmaine Owned by:
Priority: normal Milestone:
Component: web Keywords: xmlrpc datetime
Cc: khorn Branch: branches/xmlrpc-datetime-3219
(diff, github, buildbot, log)
Author: chadmaine, exarkun Launchpad Bug:

Description

Python's xmlrpclib.loads will convert xmlrpclib.DateTime objects to datetime.datetime objects when the use_datetime arg is set to True.

I've attached a simple diff that provides this functionality for twisted.web.xmlrpc.

Attachments (3)

xmlrpc_datetime.diff (3.3 KB) - added by chadmaine 6 years ago.
xmlrpc_datetime_r28820.diff (3.0 KB) - added by chadmaine 4 years ago.
Updated against r28820
xmlrpc_datetime_r28853.diff (13.6 KB) - added by chadmaine 4 years ago.
Updated diff with exarkun recommended changes, please review

Download all attachments as: .zip

Change History (18)

Changed 6 years ago by chadmaine

comment:1 Changed 5 years ago by chadmaine

Anyone had a chance to look at this patch? We continue to patch Twisted manually to use this feature.

Chad

comment:2 Changed 5 years ago by rion

i also want this feature =)

comment:3 Changed 5 years ago by khorn

  • Cc khorn added

comment:4 Changed 5 years ago by khorn

This change seems reasonable. Does anyone else see a reason not to proceed with this? Will it conflict with anything else?

Changed 4 years ago by chadmaine

Updated against r28820

comment:5 Changed 4 years ago by exarkun

  • Keywords review added
  • Owner jknight deleted

comment:6 Changed 4 years ago by exarkun

  • Keywords review removed
  • Owner set to chadmaine

Hi Chad, thanks for the update. The patch looks pretty good, but there are a couple changes it'll need before we can include it in trunk:

  1. The new useDateTime attribute/parameter of XMLRPC and Proxy needs to be documented. Convention is to document it as an ivar on the class (rather than as a parameter to __init__). This should have been done for allowNone as well, by whoever added that.
  2. The two new features (server-side and client-side datetime support) need unit tests. You can find the existing unit tests for this module in twisted/web/test/test_xmlrpc.py.
  3. use_datetime was introduced in Python 2.5. Twisted still supports Python 2.4, so we need to do something about that case. Certainly we should not pass it to xmlrpclib if we're running on Python 2.4, as that will raise an exception. Possibly we should also raise an exception in XMLRPC and Proxy if we're running on Python 2.4 and the application passes useDateTime in, to mark this error as early as possible.
  4. If you like, it'd also be helpful to include a news fragment for this change: http://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles

Thanks again for the patch. To make sure the issue gets attention again quickly when you attach a new patch, please add the "review" keyword to it.

Changed 4 years ago by chadmaine

Updated diff with exarkun recommended changes, please review

comment:7 Changed 4 years ago by chadmaine

  • Keywords review added

comment:8 Changed 4 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/xmlrpc-datetime-3219

(In [28854]) Branching to 'xmlrpc-datetime-3219'

comment:9 Changed 4 years ago by exarkun

  • Author changed from exarkun to chadmaine
  • Owner changed from chadmaine to exarkun
  • Status changed from new to assigned

comment:10 Changed 4 years ago by exarkun

  • Keywords review removed

Thanks for the updated patch! Some comments about it:

  1. XMLRPCAllowNoneTestCase.setUp should have a docstring.
  2. You can use TestCase.addCleanup in XMLRPCAllowNoneTestCase.setUp to get rid of the listening port, instead of using tearDown. It doesn't make much difference in this case, but it does mean you can completely get rid of tearDown (which means no one needs to write a docstring for it :).
  3. Test method docstrings shouldn't start with "Test that ...". They can just describe the behavior being tested.
  4. The allowNone tests and the useDateTime tests could probably be factored into a mixin pretty easily, reducing the total amount of code needed to test these two features.
  5. The new tests need to skip if being run on Python 2.4, since they'll fail with a RuntimeError, but that's expected and correct.
  6. There should also be tests for the RuntimeError raised if this code is running on Python 2.4.
  7. Since XMLRPC is a classic class, property setters don't work on it. The check needs to be done in __setattr__.
  8. There's a mistake in the new docs for useDateTime - they say "<= 2.5" but they mean ">= 2.5".
  9. Most of the _QueryFactory docstring changes look good, but a few of the __init__ parameters don't actually end up as instance attributes, so these should stay as docs in __init__ (sorry if I wasn't clear about that).
  10. _QueryFactory is private, so it probably doesn't need to be protected against a bad value for useDateTime; the checks in XMLRPC will make sure a bad value isn't passed in.

I'm going to make these changes and re-submit for review.

comment:11 follow-up: Changed 4 years ago by exarkun

  • Author changed from chadmaine to chadmaine, exarkun
  • Keywords review added
  • Owner exarkun deleted
  • Priority changed from low to normal
  • Status changed from assigned to new

Made the above changes, someone else please review.

comment:12 in reply to: ↑ 11 Changed 4 years ago by chadmaine

Replying to exarkun:

Made the above changes, someone else please review.

Looking it over, it looks good. Thanks for your help, exarkun. I did notice the docs still have my 'Python <= 2.5' in them in a couple of places.

comment:13 Changed 4 years ago by itamar

  • Keywords review removed
  • Owner set to exarkun

Looks good, other than same comment as chadmaine (you need to replace Python <= 2.5 with >= 2.5 in a few places.)

comment:14 Changed 4 years ago by exarkun

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

(In [28880]) Merge xmlrpc-datetime-3219

Author: chadmaine, exarkun
Reviewer: exarkun, chadmaine, itamar
Fixes: #3219

Add support for representing timestamps received over XML-RPC as
datetime instances, on versions of Python where xmlrpclib supports
that.

comment:15 Changed 3 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.