Storm

Merge lp://staging/~jkakar/storm/json-property into lp://staging/storm

json-property
Merge into trunk

Proposed by Jamu Kakar on 2011-02-28

Status:

Merged

Approved by:

Thomas Herve on 2011-09-07

Approved revision:

389

Merged at revision:

399

Proposed branch:

lp://staging/~jkakar/storm/json-property

Merge into:

lp://staging/storm

Diff against target:

316 lines (+158/-14)

6 files modified

storm/compat.py (+32/-0)
storm/locals.py (+2/-2)
storm/properties.py (+5/-2)
storm/variables.py (+34/-6)
tests/properties.py (+62/-0)
tests/variables.py (+23/-4)

To merge this branch:

bzr merge lp://staging/~jkakar/storm/json-property

Medium

Fix Released

Link a bug report

Reviewer	Date Requested	Status
Thomas Herve (community)	2011-02-28	Approve on 2011-09-07
Stuart Bishop (community)	2011-02-28	Approve on 2011-03-01
Review via email: mp+51634@code.staging.launchpad.net

Description of the change

This branch introduces the following changes:

- Most of the logic in the PickleVariable code has been split out into
a new EncodedValueVariable class.

- A new JSONVariable, based on EncodedValueVariable, is available.

- A new JSON property is available.

Revision history for this message

Stuart Bishop (stub) wrote on 2011-03-01:

This all looks good.

There is a lot of cut-and-paste code in test_properties that should be shared, but that isn't an issue you need to deal with on this branch.

review: Approve

Revision history for this message

Jamu Kakar (jkakar) wrote on 2011-03-01:

Stuart:

Thanks for the review! I thought about fixing the cut-and-paste issue
in the property tests, but it would be a bit messy, so I decided to
avoid it for now.

Revision history for this message

Thomas Herve (therve) wrote on 2011-03-21:

[1] The json module is only available in 2.6, we should at least keep 2.5 compatibility for now. I suggest moving the import at the class level (so that it's only used when a person use the variable), and fallback on simplejson for 2.5. The tests should be skipped if json/simplejson is not available.

review: Needs Fixing

lp://staging/~jkakar/storm/json-property updated on 2011-09-07