Opened 11 years ago

Closed 8 years ago

#2651 enhancement closed fixed (fixed)

amp could be able to encode DateTime and Decimal

Reported by: indigo Owned by:
Priority: normal Milestone:
Component: core Keywords: amp
Cc: therve, Richard Wall Branch: branches/amp-datetime-decimal-2651
branch-diff, diff-cov, branch-cov, buildbot
Author: therve

Description

Dates and times are cool. Decimals are great.

Attachments (2)

ampext.py (4.0 KB) - added by indigo 11 years ago.
implementation of nice things
2651.patch (9.3 KB) - added by thomasvs 8 years ago.
reworked patch, integrating into amp.py, with unit tests

Download all attachments as: .zip

Change History (20)

Changed 11 years ago by indigo

Attachment: ampext.py added

implementation of nice things

comment:1 Changed 11 years ago by therve

Cc: therve added

That would be even better if tests where unit tests.

comment:2 Changed 11 years ago by Glyph

Owner: changed from Glyph to indigo

Thanks!

But yeah, as therve says, can you convert the test to a unit test and the implementation into a unified diff? This page has some useful information about requirements for integrating patches, etc.

Not all of this should be going into one file. The fixed-offset timezone, for example, should probably live in twisted.python somewhere. For that matter, maybe the right way to implement that is to bring across the excellent Time class you did for Divmod and put it into twisted.python somewhere and use that for representing time objects on the wire.

Changed 8 years ago by thomasvs

Attachment: 2651.patch added

reworked patch, integrating into amp.py, with unit tests

comment:3 Changed 8 years ago by thomasvs

Keywords: review added
Owner: indigo deleted

comment:4 Changed 8 years ago by therve

Author: therve
Branch: branches/amp-datetime-decimal-2651

(In [28860]) Branching to 'amp-datetime-decimal-2651'

comment:5 Changed 8 years ago by therve

Keywords: review removed
Owner: set to thomasvs

Looks good! I have only one comment:

  • Most test classes and test methods need a docstring.

Please provide the patch against the branch I made if psossible.

Thanks!

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

Owner: changed from thomasvs to Jean-Paul Calderone
Status: newassigned

comment:7 Changed 8 years ago by Jean-Paul Calderone

(In [28862]) Address some doc and testing issues

refs #2651

  • Clarify the docstrings for several of the ListOf combo test
  • Rename the ListOf Decimal NaN test case to follow the convention
  • Add docstrings for classes and methods which were missing them
  • Convert the Decimal NaN test to use assertTrue instead of failUnless
  • Add a test case which directly tests amp.Decimal (the other always also tested ListOf)
  • Rename amp.UTCtzinfo to amp.utc
  • Add some more tests for amp._FixedOffsetTZInfo
  • Add some specification for the format used by amp.Decimal
  • Reject all non-decimal.Decimal instances in amp.Decimal.toString
  • Fix exception raising style in amp.DateTime.fromString
  • Tweak the exception raised when a naive datetime is encountered

comment:8 Changed 8 years ago by Jean-Paul Calderone

(In [28863]) Add some more unit tests for datetime and... simplify? the implementation a bit

refs #2651

  • Add a reference to #4409 near the new XXX
  • Add direct unit tests for DateTime.toString and .fromString that should exercise all of its parsing code
  • Split the _FixedOffsetTZInfo tests off into another testcase again
  • Add some more docs for _FixedOffsetTZInfo
  • Simplify the DateTime parser to just consider offsets, not try to split up the string based on values

comment:9 Changed 8 years ago by Jean-Paul Calderone

(In [28864]) News fragment

refs #2651

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

Keywords: review added
Owner: Jean-Paul Calderone deleted
Status: assignednew

Thanks for the review Thomas. I started off just looking at docstring improvements, but I got caught up in things. :)

All my changes are linked in the three comments above.

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

Ah, also, build results.

comment:12 Changed 8 years ago by Jean-Paul Calderone

(In [28865]) Stop using Decimal.is_{qnan,snan,signed}; they're new in 2.6

refs #2651

comment:13 Changed 8 years ago by Richard Wall

Cc: Richard Wall added
Keywords: review removed
Owner: set to Richard Wall
Status: newassigned

comment:14 Changed 8 years ago by Richard Wall

Owner: changed from Richard Wall to Jean-Paul Calderone
Status: assignednew
  • Tests pass

source:branches/amp-datetime-decimal-2651/twisted/test/test_amp.py

  1. Remove comment about missing test body
    1484	        # XXX something is missing.  See #4409. 
    

source:branches/amp-datetime-decimal-2651/twisted/protocols/amp.py

  1. I may have misunderstood the NaN stuff, but surely Quiet NaN would be encoded as qNaN - in which case the NaN tests may need updating
    	2458	      - Quiet not-a-number is encoded as either C{"NaN"} or C{"-NaN"} 
     	2459	      - Signalling not-a-number is encoded as either C{"qNaN"} or C{"-qNaN"} 
    

Otherwise looks good.

comment:15 Changed 8 years ago by Jean-Paul Calderone

(In [28904]) signalling nan is sNaN not qNaN

refs #2651

comment:16 Changed 8 years ago by Jean-Paul Calderone

(In [28905]) maybe this won't conflict if we get rid of the XXX

refs #2651

comment:17 Changed 8 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [28906]) Merge amp-datetime-decimal-2651

Author: indigo, thomasvs, Zaheer Merali, exarkun Reviewer: therve, glyph, exarkun, rwall Fixes: #2651

Add AMP argument types for serializing timestamps (Python datetime instances) and decimals (Python Decimal instances).

comment:18 Changed 7 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.