Support for Decimal in twisted.spread.jelly

[james@…]

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.) =)

[therve]

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

[james@…]

Added unit test for decimal jellying

[therve]

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

[therve]

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

Refs #2920

[therve]

This is ready to review.

[exarkun]

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.

[therve]

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

Refs #2920

[therve]

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!

[exarkun]

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.

[therve]

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

Refs #2920

[therve]

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

Refs #2920

[therve]

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

[exarkun]

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.

[therve]

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

Refs #2920

[therve]

Sorry for not seeing the test. Back to review.

[exarkun]

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.

[therve]

Fixed in r22301.