Fix to allow Literal and Union via | in csp annotations #476
Conversation
Signed-off-by: Nijat Khanbabayev <nijat.khanbabayev@cubistsystematic.com>
NeejWeej
requested review from
robambalu,
AdamGlustein,
svatasoiu and
alexddobkin
as code owners
Signed-off-by: Nijat Khanbabayev <nijat.khanbabayev@cubistsystematic.com>
NeejWeej
force-pushed
the
nk/ast_fix
branch
2 times, most recently
from
235f6a0 to
902f3ce
Compare
…via pydantic Signed-off-by: Nijat Khanbabayev <nijat.khanbabayev@cubistsystematic.com>
| @@ -223,7 +246,9 @@ def _obj_from_python(cls, json, obj_type): | |||
| return obj_type(json) | |||
|
|
|||
| @classmethod | |||
| def from_dict(cls, json: dict): | |||
| def from_dict(cls, json: dict, use_pydantic: bool = False): | |||
| if use_pydantic: | |||
There was a problem hiding this comment.
Rather than an arg can we base this on whether we are using pydantic type checking ( which defaults to true now )?
My only concern is speed. If we can do some timing tests and pydantic is as fast as the current code, I would just default it to use pydantic ( and eventually remove the old impl )
There was a problem hiding this comment.
The old way is a bit permissive and there will be some inconsistencies that might be annoying to fix, so I figured itd be easier to make it a flag for now. But eventually would like to switch to pydantic if its performant enough
| def from_dict(cls, json: dict): | ||
| def from_dict(cls, json: dict, use_pydantic: bool = False): | ||
| if use_pydantic: | ||
| return cls.type_adapter().validate_python(json) | ||
| return cls._obj_from_python(json, cls) | ||
|
|
||
| def to_dict_depr(self): |
There was a problem hiding this comment.
should we have an equivalent pydantic to_dict?
There was a problem hiding this comment.
@robambalu Should we just remove to_dict_depr now?
| return cls._NORMALIZED_TYPE_MAPPING.get(CspTypingUtils.get_origin(typ), typ) | ||
| elif CspTypingUtils.is_union_type(typ): | ||
| return object | ||
| elif CspTypingUtils.is_literal_type(typ): |
There was a problem hiding this comment.
I realize we just moved code here, but looks like we can probably use normalized_type_to_actual_python_type here?
There was a problem hiding this comment.
But literal takes instances of types, not types
(so like Literal[7] versus Literal[int]) and normalized_type_to_actual_python_type doesnt seem to actually pull the true base class out except in this case for Literal
fixes #475