-
Notifications
You must be signed in to change notification settings - Fork 192
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
fix: remove python version upper bound #1015
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
db6c51c
to
eb299d8
Compare
This comment has been minimized.
This comment has been minimized.
eb299d8
to
f7ff3f8
Compare
This comment has been minimized.
This comment has been minimized.
f7ff3f8
to
1710c13
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
a90bacd
to
22272c7
Compare
This comment has been minimized.
This comment has been minimized.
22272c7
to
2d09dfc
Compare
This comment has been minimized.
This comment has been minimized.
2d09dfc
to
feeeeaa
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
feeeeaa
to
7a7e464
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
fixes #944 the main fix here is switching from `cattr.register_structure_hook` to `cattr.register_structure_hook_func`. I re-organized some of the code in an effort to reproduce the issue in the test_serialize.py suite but I could not reproduce there, only in integration tests. Now that it's a function, we don't need a generated hook per type. I further isolated the converter usage switching the serializing to use each 31/40 dedicated converter. I also switched to the newer GenConverter. setup testing against python 3.10
7a7e464
to
e627f4a
Compare
Codegen Tests 1 files 18 suites 26s ⏱️ Results for commit e627f4a. |
const forwardRef = `ForwardRef("${type.name}")` | ||
this.hooks.push( | ||
`sr.converter${this.apiRef}.register_structure_hook(\n${bump}${forwardRef}, # type: ignore\n${bump}${this.structureHookFR} # type:ignore\n)` | ||
) |
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.
no longer need a register hook per type
forward_ref_structure_hook = functools.partial( | ||
sr.forward_ref_structure_hook, globals(), sr.converter${this.apiRef} | ||
) | ||
sr.converter${this.apiRef}.register_structure_hook_func( | ||
lambda t: t.__class__ is ForwardRef, forward_ref_structure_hook | ||
) |
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.
now it's just this one new register_structure_hook_func
@@ -62,7 +62,7 @@ def init31( | |||
return methods31.Looker31SDK( | |||
auth_session.AuthSession(settings, transport, serialize.deserialize31, "3.1"), | |||
serialize.deserialize31, | |||
serialize.serialize, | |||
serialize.serialize31, |
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.
let's keep the cattrs converters separate both on input and output
def _tr_data_keys(data): | ||
"""Map top level json keys to model property names. | ||
|
||
Currently this translates reserved python keywords like "from" => "from_" | ||
""" | ||
for reserved in keyword.kwlist: | ||
if reserved in data and isinstance(data, dict): | ||
data[f"{reserved}_"] = data.pop(reserved) | ||
return data | ||
|
||
|
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.
moved to new hooks.py file
Map reserved_ words in models to correct json field names. | ||
Also handle stripping None fields from dict while setting | ||
EXPLICIT_NULL fields to None so that we only send null | ||
in the json for fields the caller set EXPLICIT_NULL on. | ||
""" | ||
data = cattr.global_converter.unstructure_attrs_asdict(api_model) | ||
for key, value in data.copy().items(): | ||
if value is None: | ||
del data[key] | ||
elif value == model.EXPLICIT_NULL: | ||
data[key] = None | ||
# bug here: in the unittests cattrs unstructures this correctly | ||
# as an enum calling .value but in the integration tests we see | ||
# it doesn't for WriteCreateQueryTask.result_format for some reason | ||
# Haven't been able to debug it fully, so catching and processing | ||
# it here. | ||
elif isinstance(value, enum.Enum): | ||
data[key] = value.value | ||
for reserved in keyword.kwlist: | ||
if f"{reserved}_" in data: | ||
data[reserved] = data.pop(f"{reserved}_") | ||
return data | ||
|
||
|
||
DATETIME_FMT = "%Y-%m-%dT%H:%M:%S.%f%z" | ||
if sys.version_info < (3, 7): | ||
from dateutil import parser | ||
|
||
def datetime_structure_hook( | ||
d: str, t: Type[datetime.datetime] | ||
) -> datetime.datetime: | ||
return parser.isoparse(d) | ||
|
||
else: | ||
|
||
def datetime_structure_hook( | ||
d: str, t: Type[datetime.datetime] | ||
) -> datetime.datetime: | ||
return datetime.datetime.strptime(d, DATETIME_FMT) | ||
|
||
|
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.
moved to new hooks.py file
@@ -55,7 +55,7 @@ | |||
package_data={"looker_sdk": ["py.typed", "looker_sdk/looker-sample.ini"]}, | |||
packages=find_packages(), | |||
# restrict python to <=3.9.9 due to https://github.com/looker-open-source/sdk-codegen/issues/944 | |||
python_requires=">=3.6, <=3.9.9", | |||
python_requires=">=3.6", |
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.
up up and away
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.
I think we can also delete the comment now in a followup
@@ -95,6 +95,7 @@ class Model(ml.Model): | |||
list_model_no_refs1: Sequence["ModelNoRefs1"] | |||
opt_enum1: Optional["Enum1"] = None | |||
opt_model_no_refs1: Optional["ModelNoRefs1"] = None | |||
list_opt_model_no_refs1: Optional[Sequence["ModelNoRefs1"]] = None |
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.
thought I saw some odd behavior so I added this version of type to make sure cattrs gets it right
Python Tests 11 files 11 suites 2m 37s ⏱️ Results for commit e627f4a. |
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.
LGTM. Thanks for the inline comments
fixes #944
the main fix here is switching from
cattr.register_structure_hook
tocattr.register_structure_hook_func
. I re-organized some of the codein an effort to reproduce the issue in the test_serialize.py suite but
I could not reproduce there, only in integration tests.
Now that it's a function, we don't need a generated hook per type.
I further isolated the converter usage switching the serializing to use
each 31/40 dedicated converter.
I also switched to the newer GenConverter.