-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[improvement] add support for hashing / settype + more tests #39
Changes from 5 commits
71a35ef
0562c30
7c7fdc6
042826e
aebed6d
64ed9b3
8c0c820
7256727
42770eb
dc8eec5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,5 +33,6 @@ | |
'RequestsClient', | ||
'Service', | ||
'ServiceConfiguration', | ||
'SetType', | ||
'SslConfiguration', | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,4 +27,5 @@ | |
'DictType', | ||
'ListType', | ||
'OptionalType', | ||
'SetType' | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,7 @@ | |
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
from typing import List, Dict, Type, Any, Union | ||
from typing import List, Dict, Type, Any, Union, FrozenSet | ||
from enum import Enum | ||
|
||
from .case import to_snake_case | ||
|
@@ -24,7 +24,8 @@ class ConjureType(object): | |
|
||
|
||
DecodableType = Union[ | ||
int, float, bool, str, ConjureType, List[Any], Dict[Any, Any] | ||
int, float, bool, str, ConjureType, | ||
List[Any], Dict[Any, Any], FrozenSet[Any] | ||
] | ||
|
||
|
||
|
@@ -36,6 +37,14 @@ def __init__(self, item_type): | |
self.item_type = item_type | ||
|
||
|
||
class SetType(ConjureType): | ||
item_type = None # type: Type[DecodableType] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like we're pretty inconsistent elsewhere, but fields should probably be underscored with a |
||
|
||
def __init__(self, item_type): | ||
# type: (Type[DecodableType]) -> None | ||
self.item_type = item_type | ||
|
||
|
||
class DictType(ConjureType): | ||
key_type = None # type: Type[DecodableType] | ||
value_type = None # type: Type[DecodableType] | ||
|
@@ -78,6 +87,11 @@ def _fields(cls): | |
name to the field definition""" | ||
return {} | ||
|
||
def __hash__(self): | ||
values_tuple = tuple(self._fields().values()) | ||
keys_tuple = tuple(self._fields().keys()) | ||
return hash((values_tuple, keys_tuple)) | ||
|
||
def __eq__(self, other): | ||
# type: (Any) -> bool | ||
if not isinstance(other, self.__class__): | ||
|
@@ -121,6 +135,12 @@ def _options(cls): | |
to the field definition for that type""" | ||
return {} | ||
|
||
def __hash__(self): | ||
values = tuple([getattr(self, attr) for | ||
attr, field_def in | ||
self._options().items()]) | ||
return hash(values) | ||
|
||
def __eq__(self, other): | ||
# type: (Any) -> bool | ||
if not isinstance(other, self.__class__): | ||
|
@@ -129,9 +149,9 @@ def __eq__(self, other): | |
assert isinstance(other, ConjureUnionType) | ||
|
||
pythonic_sanitized_identifier = \ | ||
sanitize_identifier(to_snake_case(self.type)) | ||
sanitize_identifier(to_snake_case(self._type)) | ||
|
||
return other.type == self.type and \ | ||
return other._type == self._type and \ | ||
getattr(self, pythonic_sanitized_identifier) == \ | ||
getattr(other, pythonic_sanitized_identifier) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,10 +19,11 @@ | |
ConjureUnionType, | ||
DictType, | ||
ListType, | ||
OptionalType | ||
OptionalType, | ||
SetType | ||
) | ||
from typing import Optional | ||
from typing import Dict, Any, List | ||
from typing import Dict, Any, List, FrozenSet | ||
import inspect | ||
import json | ||
|
||
|
@@ -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] = [] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this not be a frozenset? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i dont think so? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it should. otherwise the actual type of a set member in an object will vary depending on the wire format of the response.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh misunderstood which thing you were suggesting swapping to frozen set. you are correct There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed and added a test |
||
elif isinstance(field_definition.field_type, DictType): | ||
deserialized[python_arg_name] = {} | ||
elif isinstance(field_definition.field_type, OptionalType): | ||
|
@@ -99,7 +102,10 @@ def decode_conjure_union_type(cls, obj, conjure_type): | |
|
||
deserialized = {} # type: Dict[str, Any] | ||
if type_of_union not in obj or obj[type_of_union] is None: | ||
cls.check_null_field(obj, deserialized, conjure_field_definition) | ||
cls.check_null_field(obj, | ||
deserialized, | ||
attr, | ||
conjure_field_definition) | ||
else: | ||
value = obj[type_of_union] | ||
field_type = conjure_field_definition.field_type | ||
|
@@ -174,6 +180,24 @@ def decode_list(cls, obj, element_type): | |
|
||
return list(map(lambda x: cls.do_decode(x, element_type), obj)) | ||
|
||
@classmethod | ||
def decode_set(cls, obj, element_type): | ||
# type: (List[Any], ConjureTypeType) -> FrozenSet[Any] | ||
"""Decodes json into a frozenset, handling conversion of the elements. | ||
|
||
Args: | ||
obj: the json object to decode | ||
element_type: a class object which is the conjure type of | ||
the elements in this list. | ||
Returns: | ||
A python frozenset where the elements are instances of type | ||
element_type. | ||
""" | ||
if not isinstance(obj, list): | ||
raise Exception("expected a python list") | ||
|
||
return frozenset(map(lambda x: cls.do_decode(x, element_type), obj)) | ||
|
||
@classmethod | ||
def decode_optional(cls, obj, object_type): | ||
# type: (Optional[Any], ConjureTypeType) -> Optional[Any] | ||
|
@@ -244,6 +268,9 @@ def do_decode(cls, obj, obj_type): | |
elif isinstance(obj_type, OptionalType): | ||
return cls.decode_optional(obj, obj_type.item_type) | ||
|
||
elif isinstance(obj_type, SetType): | ||
return cls.decode_set(obj, obj_type.item_type) | ||
|
||
return cls.decode_primitive(obj, obj_type) | ||
|
||
def decode(self, obj, obj_type): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
# (c) Copyright 2018 Palantir Technologies Inc. All rights reserved. | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
import pytest | ||
from conjure_python_client import ConjureDecoder, ConjureEnumType | ||
|
||
|
||
class TestEnum(ConjureEnumType): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could we use a generated enum instead of a hand rolled one? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
A = 1 | ||
B = 2 | ||
C = 3 | ||
|
||
def test_enum_decode(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could we also add some negative test cases? |
||
decoded_A = ConjureDecoder().read_from_string("\"A\"", TestEnum) | ||
decoded_B = ConjureDecoder().read_from_string("\"B\"", TestEnum) | ||
decoded_A2 = ConjureDecoder().read_from_string("\"A\"", TestEnum) | ||
assert decoded_A != decoded_B | ||
assert decoded_A == decoded_A2 | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: white space |
||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
# (c) Copyright 2018 Palantir Technologies Inc. All rights reserved. | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
import pytest | ||
from conjure_python_client import ConjureDecoder, ConjureEncoder, SetType | ||
|
||
|
||
def test_set_with_well_typed_items_decodes(): | ||
decoded = ConjureDecoder().read_from_string("[1,2,3]", SetType(int)) | ||
assert type(decoded) is frozenset | ||
assert type(list(decoded)[0]) is int | ||
|
||
|
||
def test_set_with_one_badly_typed_item_fails(): | ||
with pytest.raises(Exception): | ||
ConjureDecoder().read_from_string("""[1,"two",3]""", SetType(int)) | ||
|
||
|
||
def test_set_with_no_items_decodes(): | ||
decoded = ConjureDecoder().read_from_string("[]", SetType(int)) | ||
assert type(decoded) is frozenset | ||
|
||
|
||
def test_set_from_json_object_fails(): | ||
with pytest.raises(Exception): | ||
ConjureDecoder().read_from_string("{}", SetType(int)) | ||
|
||
|
||
def test_set_does_not_decode_from_json_null(): | ||
with pytest.raises(Exception): | ||
ConjureDecoder().read_from_string("null", SetType(int)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this a json null or the string null? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. json null |
||
|
||
|
||
def test_set_does_not_decode_from_json_string(): | ||
with pytest.raises(Exception): | ||
ConjureDecoder().read_from_string("\"hello\"", SetType(int)) | ||
|
||
def test_set_encoder(): | ||
encoded = ConjureEncoder.do_encode(frozenset([5,6,7])) | ||
assert type(encoded) is list | ||
encoded = ConjureEncoder.do_encode(set([5,6,7])) | ||
assert type(encoded) is list |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
# (c) Copyright 2018 Palantir Technologies Inc. All rights reserved. | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
import pytest | ||
from conjure_python_client import ConjureDecoder, ListType, ConjureUnionType, ConjureFieldDefinition | ||
|
||
class TestUnion(ConjureUnionType): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here, use a generated union instead of a hand rolled one |
||
_a = None | ||
_b = None | ||
_c = None | ||
|
||
@classmethod | ||
def _options(cls): | ||
return { | ||
'a': ConjureFieldDefinition('A', str), | ||
'b': ConjureFieldDefinition('B', int), | ||
'c': ConjureFieldDefinition('C', ListType(int)) | ||
} | ||
|
||
def __init__(self, a=None, b=None, c=None): | ||
if a is not None: | ||
self._a = a | ||
self._type = 'A' | ||
elif b is not None: | ||
self._b = b | ||
self._type = 'B' | ||
elif c is not None: | ||
self._c = c | ||
self._type = 'C' | ||
|
||
@property | ||
def a(self): | ||
return self._a | ||
|
||
@property | ||
def b(self): | ||
return self._b | ||
|
||
@property | ||
def c(self): | ||
return self._c | ||
|
||
|
||
def test_union_decoder(): | ||
decoded_A = ConjureDecoder().read_from_string('{"type":"A", "A": "foo"}', TestUnion) | ||
decoded_B = ConjureDecoder().read_from_string('{"type":"B", "B": 5}', TestUnion) | ||
decoded_A2 = ConjureDecoder().read_from_string('{"type":"A", "A": "bar"}', TestUnion) | ||
decoded_C = ConjureDecoder().read_from_string('{"type":"C"}', TestUnion) | ||
assert type(decoded_A) is TestUnion | ||
assert type(decoded_B) is TestUnion | ||
assert type(decoded_C) is TestUnion | ||
assert decoded_A != decoded_B | ||
assert decoded_A != decoded_A2 | ||
assert decoded_C != decoded_A | ||
assert not decoded_C.c | ||
|
||
def test_invalid_decode(): | ||
with pytest.raises(Exception): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a more specific error we can expect? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nope. the code just throws Exception at the moment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this code is copied from existing tests |
||
decoded_invalid = ConjureDecoder().read_from_string('{"type":"A", "B": "bar"}', TestUnion) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we also define
__slots__
for the class to reduce memory overhead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be done in a follow up along with the other type classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like premature optimization to me