-
Notifications
You must be signed in to change notification settings - Fork 27
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
Typed nodes and edges #1820
Typed nodes and edges #1820
Conversation
a148128
to
6626e78
Compare
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.
Very nice! I really like this!
Would be great with an example in the docs aswell! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1820 +/- ##
==========================================
+ Coverage 92.68% 92.75% +0.06%
==========================================
Files 121 122 +1
Lines 17950 18188 +238
==========================================
+ Hits 16637 16870 +233
- Misses 1313 1318 +5
|
@@ -389,7 +401,16 @@ def __contains__(self, attr: str) -> bool: | |||
self.__raise_if_non_singular_source(attr) | |||
|
|||
def dump(self, camel_case: bool = True) -> dict[str, Any]: | |||
dumped = super().dump(camel_case) |
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.
We cannot call the default dump method, as the TypedNode/Edge, will have properties directly on the class, which the default method just serialize in place.
The contained type was being inferred as Any
They only make sense if doing retrieving raw Nodes and Edges
Generic parameters and defaults don't mix well (see python/mypy#3737) so we need to use overloads instead, or InstancesResult is always InstancesResult[Any, Any]. Overloads don't work on retrieve() due to defaults and ordering of params, so we create two new methods that can be safely typed.
Then we can at least stay consistent within the DM client
@@ -77,54 +70,6 @@ def __init__( | |||
self.description = description | |||
|
|||
|
|||
class DataModelingInstancesList(WriteableCogniteResourceList, Generic[T_WriteClass, T_WritableCogniteResource], ABC): |
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.
Probably need to add an alias here.
Description
Please describe the change you have made.
Checklist:
If a new method has been added it should be referenced in cognite.rst in order to generate docs based on its docstring.