[improvement] add support for hashing / settype + more tests #39

derenrich · 2019-01-08T03:50:27Z

Before this PR

Comparison of Union types was broken
No tests for enum or union types
No way to represent Sets separate from Lists

After this PR

This PR does a few things:

fixes and tests ConjureType should include __hash__ implementation #17
adds tests for the union and enum types which had bugs before
adds a SetType for the eventual native support for frozenset to resolve Generate a real Python 'set' for the Conjure set<T> type conjure-python#27 later

ferozco

Although this change is not a break, adopting this change in conjure-python would be since we're changing the request/response types for sets. I think long term this is the right thing to do, but i'm unsure if it is the right time to have cause a major rev. @ahggns for thoughts

ferozco · 2019-01-08T21:23:50Z

test/serde/test_decode_enum.py

+    decoded_A2 = ConjureDecoder().read_from_string("\"A\"", TestEnum)
+    assert decoded_A != decoded_B
+    assert decoded_A == decoded_A2
+


nit: white space

ferozco · 2019-01-08T21:24:39Z

test/serde/test_decode_enum.py

+    B = 2
+    C = 3
+
+def test_enum_decode():


could we also add some negative test cases?

ferozco · 2019-01-08T21:24:57Z

test/serde/test_decode_enum.py

+from conjure_python_client import ConjureDecoder, ConjureEnumType
+
+
+class TestEnum(ConjureEnumType):


could we use a generated enum instead of a hand rolled one?

ferozco · 2019-01-08T21:27:33Z

conjure_python_client/_lib/types.py

@@ -36,6 +37,14 @@ def __init__(self, item_type):
        self.item_type = item_type


+class SetType(ConjureType):
+    item_type = None  # type: Type[DecodableType]


could we also define __slots__ for the class to reduce memory overhead?

Could be done in a follow up along with the other type classes

seems like premature optimization to me

ferozco · 2019-01-08T21:28:41Z

conjure_python_client/_serde/decoder.py

@@ -62,6 +63,8 @@ def check_null_field(
            cls, obj, deserialized, python_arg_name, field_definition):
        if isinstance(field_definition.field_type, ListType):
            deserialized[python_arg_name] = []
+        elif isinstance(field_definition.field_type, SetType):
+            deserialized[python_arg_name] = []


should this not be a frozenset?

i dont think so?

it should. otherwise the actual type of a set member in an object will vary depending on the wire format of the response.

The type will be frozenset if the collection is present ex: { "foo":[1,2] }

The type will be array if the collection is absent. ex: {}, { "foo": null }

oh misunderstood which thing you were suggesting swapping to frozen set. you are correct

fixed and added a test

ferozco · 2019-01-08T21:30:55Z

test/serde/test_decode_set.py

+
+def test_set_does_not_decode_from_json_null():
+    with pytest.raises(Exception):
+        ConjureDecoder().read_from_string("null", SetType(int))


is this a json null or the string null?

ferozco · 2019-01-08T21:31:36Z

test/serde/test_decode_union.py

+import pytest
+from conjure_python_client import ConjureDecoder, ListType, ConjureUnionType, ConjureFieldDefinition
+
+class TestUnion(ConjureUnionType):


same here, use a generated union instead of a hand rolled one

ahggns

In agreement that this is directionally correct, and the use of proper set types is unlikely to break existing code which was mostly treating lists as sets anyway.

As far as breaks go I think we have a handful of other breaks we'd been considering and it'd probably be better to do a bunch at once so we can be more watchful of the rollout.

ahggns · 2019-01-09T10:30:58Z

test/serde/test_decode_union.py

+    assert not decoded_C.c
+
+def test_invalid_decode():
+    with pytest.raises(Exception):


Is there a more specific error we can expect?

nope. the code just throws Exception at the moment

this code is copied from existing tests

ahggns · 2019-01-09T10:35:22Z

conjure_python_client/_lib/types.py

@@ -36,6 +37,14 @@ def __init__(self, item_type):
        self.item_type = item_type


+class SetType(ConjureType):
+    item_type = None  # type: Type[DecodableType]


It looks like we're pretty inconsistent elsewhere, but fields should probably be underscored with a @property method for access.

derenrich · 2019-01-10T00:25:31Z

in response to

i'm unsure if it is the right time to have cause a major

I think we should merge this now and then discuss when to cause a break in the conjure-python repo

derenrich · 2019-01-10T19:18:52Z

good to go?

ferozco · 2019-01-31T18:06:13Z

Sorry, this totally slipped through the cracks. I think getting this in would be great!

derenrich · 2019-01-31T18:55:15Z

cool. did i miss any changes you requested? the PR is still disapproved by you @ferozco

…39)" This reverts commit 2871176.

@geremih

…39)" (#52) Reverts #39 reported by @geremih - seems like the conjure bean type's hashing is not stable! Just doing a quick revert to make sure latest is known good, then we can revisit with a proper fix here.

add support for hashing / settype + more tests

71a35ef

derenrich requested a review from a team as a code owner January 8, 2019 03:50

Daniel Erenrich added 4 commits January 7, 2019 19:52

bugfix

0562c30

remove debug

7c7fdc6

fix style

042826e

more linting

aebed6d

ferozco suggested changes Jan 8, 2019

View reviewed changes

ahggns reviewed Jan 9, 2019

View reviewed changes

Daniel Erenrich added 2 commits January 9, 2019 16:19

respond to comments

64ed9b3

lint fix

8c0c820

fix empty set decoding and add test

7256727

derenrich and others added 2 commits January 31, 2019 10:44

Merge branch 'develop' into feature/set-type-tests-hash

42770eb

type error on parse enum non-string

dc8eec5

ferozco approved these changes Jan 31, 2019

View reviewed changes

iamdanfox added the merge when ready label Feb 1, 2019

bulldozer-bot bot merged commit 2871176 into palantir:develop Feb 1, 2019

iamdanfox added a commit that referenced this pull request Mar 6, 2019

Revert "[improvement] add support for hashing / settype + more tests (#…

cc7182d

…39)" This reverts commit 2871176.

iamdanfox mentioned this pull request Mar 6, 2019

Revert "[improvement] add support for hashing / settype + more tests" #52

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[improvement] add support for hashing / settype + more tests #39

[improvement] add support for hashing / settype + more tests #39

derenrich commented Jan 8, 2019

ferozco left a comment

ferozco Jan 8, 2019

ferozco Jan 8, 2019

ferozco Jan 8, 2019

derenrich Jan 10, 2019

ferozco Jan 8, 2019

ferozco Jan 8, 2019

derenrich Jan 9, 2019

ferozco Jan 8, 2019

derenrich Jan 9, 2019

ferozco Jan 10, 2019

derenrich Jan 10, 2019

derenrich Jan 10, 2019

ferozco Jan 8, 2019

derenrich Jan 9, 2019

ferozco Jan 8, 2019

ahggns left a comment

ahggns Jan 9, 2019

derenrich Jan 9, 2019

derenrich Jan 9, 2019

ahggns Jan 9, 2019

derenrich commented Jan 10, 2019

derenrich commented Jan 10, 2019

ferozco commented Jan 31, 2019

derenrich commented Jan 31, 2019

		from conjure_python_client import ConjureDecoder, ConjureEnumType


		class TestEnum(ConjureEnumType):

[improvement] add support for hashing / settype + more tests #39

[improvement] add support for hashing / settype + more tests #39

Conversation

derenrich commented Jan 8, 2019

Before this PR

After this PR

ferozco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahggns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derenrich commented Jan 10, 2019

derenrich commented Jan 10, 2019

ferozco commented Jan 31, 2019

derenrich commented Jan 31, 2019