Ticket #2920 (closed enhancement: fixed )

Opened 2 years ago

Last modified 1 year ago

Support for Decimal in twisted.spread.jelly

Reported by: james@tri-tech.com Assigned to: therve
Type: enhancement Priority: highest
Milestone: Component: pb
Keywords: Cc: exarkun, therve
Branch: branches/decimal-jelly-2920 Author: therve
Launchpad Bug:

Description

I've added support for Decimal() as a built in immutable type to the jelly module.

In the absence of the decimal module (on either end) support should gracefully degrade. (Not that I've actually tested that premise.) =)

Attachments

decimal_jelly.patch (2.2 kB) - added by james@tri-tech.com 2 years ago.
Added unit test for decimal jellying

Change History

  2007-11-21 09:14:33+00:00 changed by therve

  • cc set to review
  • owner changed from warner to james@tri-tech.com

Thanks for your contribution, that's a first good start, but it won't get in without unittests. Would you volunteer to add them?

  2007-11-21 16:22:10+00:00 changed by james@tri-tech.com

  • status changed from new to assigned

  2007-11-21 18:20:02+00:00 changed by james@tri-tech.com

  • attachment decimal_jelly.patch added

Added unit test for decimal jellying

  2007-11-21 18:21:38+00:00 changed by james@tri-tech.com

  • owner changed from james@tri-tech.com to therve
  • status changed from assigned to new

  2007-11-22 10:13:09+00:00 changed by therve

  • branch set to branches/decimal-jelly-2920
  • author set to therve

(In [21938]) Branching to 'decimal-jelly-2920'

  2007-11-22 10:32:35+00:00 changed by therve

(In [21939]) Add decimal support, with tests.

Refs #2920

  2007-11-22 10:42:41+00:00 changed by therve

  • cc deleted
  • keywords set to review
  • owner deleted
  • priority changed from normal to highest

This is ready to review.

  2007-12-28 04:30:07+00:00 changed by exarkun

  • keywords deleted
  • owner set to therve

Unpersistable seems to be systematically misused throughout jelly.py. The docstring claims it represents something related to persisting, however it is only used by unjellying code, not jellying code. So it's clearly related to unpersisting. Next, the argument to its initializer is "reason" which is documented as being "a descriptive ... string". One existing use of this respects this, but two others don't; the new use also doesn't. There's no test for the value passed to this initializer by the new code, either.

The whitespace for the def _unjelly_decimal line is weird/wrong.

I'm really reaching here. This patch seems pretty good, as far as it goes. Jelly could clearly use a bit of a cleanup, but that's probably a separate thing.

  2007-12-28 12:28:35+00:00 changed by therve

(In [22162]) Fix Unpersitable usage, add a test for decimal case, whitespaces.

Refs #2920

  2007-12-28 12:35:00+00:00 changed by therve

  • cc set to exarkun, therve
  • keywords set to review
  • owner deleted

I wasn't sure about the good thing to do, so I fixed the Unpersistable calls to have a descriptive string.

Regarding the jelly cleanup, I'm willing to do it but I prefer to make in a independent branch. Back to review!

  2008-01-02 01:22:30+00:00 changed by exarkun

  • keywords deleted
  • owner set to therve

Thanks for the Unpersistable cleanups. Doing anything else in an independent branch is fine with me.

Is there any direct test coverage for the change to SecurityOptions.__init__? I guess being able to jelly decimals at all means the security options is allowing them through, but it'd be nice to be explicit about it, and some tests for the opposite outcome (ie, rejecting decimals) would be nice too.

Last time I looked at this I thought a bit about what the consequences of using the base-10 string representation of the decimal object might be. The main (perhaps only) drawback is the size. I didn't think of a better solution then. This time around, I poked a bit more. What do you think of something like this:

def jellydecimal(d):
    sign, guts, exponent = d.as_tuple()
    value = reduce(lambda left, right: left * 10 + right, guts)
    if sign:
        value = -value
    return ['decimal', value, exponent]

This produces output which is larger by 1 byte for values between 0 and 9 inclusive, output which is the same size for values between 10 and 99 inclusive, and smaller output for larger values. Negative values have similar results. I'd guess it's friendlier to the CPU as well, although that might all get lost when banana does into int2b128. The downside is that it's more complex than str(d).

A reason to think about this now is that we want to preserve compatibility in the wire protocol. We could just pick a new type name later, but that complicates the protocol a bit and it'd be nice to avoid if possible.

  2008-01-02 14:09:56+00:00 changed by therve

(In [22249]) Try a smarter way to jelly decimail.

Refs #2920

  2008-01-02 15:00:39+00:00 changed by therve

(In [22252]) Add tests for security options for decimal.

Refs #2920

  2008-01-02 15:13:18+00:00 changed by therve

  • keywords set to review
  • owner deleted

Thanks a lot for the great review. It should be good now.

  2008-01-03 02:39:22+00:00 changed by exarkun

  • keywords deleted
  • owner set to therve

test_decimalMissing is a bit invalid now, even though it passes. :)

Test coverage for negatives would be good, too. It might be nice to use the literal jellied form somehow in test_decimal (that is, something like the list used in test_decimalMissing) to make sure things really turn out how we expect. This list could also be shared with test_decimalMissing and that way avoid the possibility of passing in bogus data without noticing.

  2008-01-03 14:04:34+00:00 changed by therve

(In [22255]) Fix a test, add a new one to check data.

Refs #2920

  2008-01-06 10:08:54+00:00 changed by therve

  • keywords set to review
  • owner deleted

Sorry for not seeing the test. Back to review.

  2008-01-06 21:53:40+00:00 changed by exarkun

  • keywords deleted
  • owner set to therve

Looks pretty good. I'd adjust the test_decimal and test_decimalUnjelly docstrings to read more like the test_decimalMissing docstring:

"""
Jellying L{decimal.Decimal} instances and then unjellying the result should produce
objects which represent the values of the original inputs.
"""

and

"""
Unjellying the s-expressions produced by jelly for L{decimal.Decimal} instances should
result in L{decimal.Decimal} instances with the values represented by the s-expressions.
.
This test also verifies that C{self.decimalData} contains valid jellied data.  This is
important since L{test_decimalMissing} re-uses C{self.decimalData} and is expected to
be unable to produce L{decimal.Decimal} instances even though the s-expression correctly
represents a list of them.
"""

In test_decimalMissing and test_decimalSecurity, I'd also replace it with L{jelly.unjelly} for clarity.

The rest looks good. Merge when ready.

  2008-01-09 09:02:34+00:00 changed by therve

  • status changed from new to closed
  • resolution set to fixed

Fixed in r22301.

Note: See TracTickets for help on using tickets.