refactor[next]: use eve.datamodel for types #1750
Conversation
| @@ -108,7 +108,7 @@ def visit_Subscript(self, node: past.Subscript, **kwargs: Any) -> past.Subscript | |||
| value = self.visit(node.value, **kwargs) | |||
| return past.Subscript( | |||
| value=value, | |||
| slice_=self.visit(node.slice_, **kwargs), | |||
| slice_=node.slice_, # TODO: I don't think we are using this type | |||
There was a problem hiding this comment.
| slice_=node.slice_, # TODO: I don't think we are using this type | |
| slice_=node.slice_, |
check and remove
There was a problem hiding this comment.
We don't have a type for past.Slice, but the visit here also types & validates its children (e.g 1+1. in a slice bound would error here). For that reason I consider unconditionally visiting all children a good approach. The role of ts.DeferredType and None in the type system is however not well specified. I would postpone that discussion to the point where we also have time to think about generics / "templated" / partial types, because the discussion is related.
There was a problem hiding this comment.
What are you proposing to do? Accept None in TupleType elements, this proposed solution or add visit_Slice and return DeferredType.
There was a problem hiding this comment.
Ah I overlooked that slice_ is a tuple of slices (maybe the attr name is not so great?). visit_Slice with DeferredType makes sense to me even though this would add another violation of all-type-attrs-of-types-are-concrete rule of a ts.TypeSpec. Otherwise introduction of a slice type makes sense to me too.
| # TODO: or DimensionType is a DataType | ||
| types: list[DataType | DimensionType | DeferredType] | ||
| # TODO validate DeferredType constraints |
| @@ -108,7 +108,7 @@ def visit_Subscript(self, node: past.Subscript, **kwargs: Any) -> past.Subscript | |||
| value = self.visit(node.value, **kwargs) | |||
| return past.Subscript( | |||
| value=value, | |||
| slice_=self.visit(node.slice_, **kwargs), | |||
| slice_=node.slice_, # TODO: I don't think we are using this type | |||
There was a problem hiding this comment.
We don't have a type for past.Slice, but the visit here also types & validates its children (e.g 1+1. in a slice bound would error here). For that reason I consider unconditionally visiting all children a good approach. The role of ts.DeferredType and None in the type system is however not well specified. I would postpone that discussion to the point where we also have time to think about generics / "templated" / partial types, because the discussion is related.
| return constraint | ||
| case ts.TypeSpec() as concrete_type: | ||
| return concrete_type.__class__ | ||
| if isinstance(symbol_type, ts.DeferredType): |
There was a problem hiding this comment.
Just a question: why move away from the match pattern matching?
There was a problem hiding this comment.
as discussed, datamodels don't implement __match_args__
|
|
||
| def __init_subclass__(cls) -> None: | ||
| cls.__hash__ = TypeSpec.__hash__ # type: ignore[method-assign] | ||
| class TypeSpec(eve_datamodels.DataModel, kw_only=False, frozen=True): ... # type: ignore[call-arg] |
There was a problem hiding this comment.
The previous implementation used a custom hash definition which would work even for non frozen classes and classes with mutable attributes (e.g. lists). I think that was made just for convenience, because it doesn't follow the python semantics for hash, but the new implementation doesn't replicate the previous behavior. How does it work? It is not really needed to hash instances with mutable attributes like lists or none of the subclasses contains mutable attributes.
There was a problem hiding this comment.
Since there is no issue (in our tests), I assume it's fine.
Co-authored-by: Till Ehrengruber <[email protected]>
No description provided.