couchdb-python icon indicating copy to clipboard operation
couchdb-python copied to clipboard

Support TupleFIeld for tuples with fixed types

Open djc opened this issue 10 years ago • 10 comments

From [email protected] on February 12, 2011 00:08:09

The attached patch implements support for this, because I needed it in my project (which uses paisley which now uses a copy of mapping.py)

Note: when I run doctests with

nosetests --with-doctest couchdb/mapping.py

I get errors for ListField, TupleField and ViewField.

Without the patch I only get the errors for ViewField.

Am I running the doctests right ? I don't see why adding TupleField should have an effect on the ListField doctest.

Original issue: http://code.google.com/p/couchdb-python/issues/detail?id=165

djc avatar Jul 12 '14 14:07 djc

From [email protected] on February 11, 2011 15:08:48

Sorry, I opened as Defect and should be Enhancement I guess - no idea how to change it though.

djc avatar Jul 12 '14 14:07 djc

From [email protected] on February 11, 2011 16:23:32

sorry, that patch was merged wrong after updating my python-couchdb checkout.

Here's a better patch, with working doctests.

It did bring to light something that looks wrong with the ListField implementation:

  • why shouldn't author and comment be returned as unicode in its doctest ?
  • why does time get returned as a string instead of a datetime ?

Compare to e.g. the DictField doctest where name and email get returned as unicode too.

The TupleField implementation is using the field's _to_python implementation, which looks like the right thing to do me.

Attachment: mapping-tuple.patch

djc avatar Jul 12 '14 14:07 djc

From djc.ochtman on February 28, 2011 04:13:24

I'm not sure about this one, though I'm leaning towards including it. On the one hand, we can't actually distinguish between tuples and lists from the JSON data, so it's a bit of guess work. On the other hand, it seems like a useful thing to have for heterogeneously typed JSON lists, which are as useful as Python's tuple.

Matt, what do you think?

Labels: -Type-Defect Type-Enhancement

djc avatar Jul 12 '14 14:07 djc

From [email protected] on February 28, 2011 06:18:43

I have no real issue with including a tuple type in the mapping schema. The Python tuple <-> JSON array conversion is not really any different than a Python datetime.date <-> JSON string conversion - they're both just mechanisms to serialise native Python types to JSON.

Quickly scanning the patch, I think it looks generally good. Just a couple of minor points:

  1. Should the default be [], the empty list? A (None, None, ...) tuple seems slightly better. Or maybe just None, although that would not be consistent with DictField and ListField.
  2. Should it check len(self.fields) == len(values) to ensure that items are never discarded?

djc avatar Jul 12 '14 14:07 djc

From djc.ochtman on February 28, 2011 07:24:59

Yeah, I think a (None, None, ...) default would be better. Checking the length also sounds like a good safeguard.

djc avatar Jul 12 '14 14:07 djc

From [email protected] on April 09, 2011 10:46:49

Adding new version of patch taking into account your suggestions.

Attachment: ma

djc avatar Jul 12 '14 14:07 djc

From kxepal on April 09, 2011 13:37:15

Seems that attachment is broken.

ma 0 bytes

djc avatar Jul 12 '14 14:07 djc

From [email protected] on April 10, 2011 06:45:59

Not sure how that happened, sorry!

Attachment: mapping.patch

djc avatar Jul 12 '14 14:07 djc

From [email protected] on May 19, 2011 06:44:07

Hi guys,

could someone please look at this patch ? It's straightforward to merge.

Thomas

djc avatar Jul 12 '14 14:07 djc

From kxepal on May 21, 2011 03:34:33

My own suggestions:

  • replace AssertionError's with ValueError
  • handle situation then TupleField would be initialized in same way as ListField (that's intuitive error)
  • may be raise ValueError for such initializations: TupleField([IntegerField, 1, None, 3, '...', 5]) because it breaks _to_python / _to_json convertations? Or leave constants as is, but I suppose that would be bad solution.
  • tests(:

djc avatar Jul 12 '14 14:07 djc