fix: instance type not possible to reset to null #1970
Conversation
There was a problem hiding this comment.
edge.type cannot be set to null - it is non-nullable. So if omitted it should not be passed as null. I'm also not certain we'll want omitted meaning explicit null for node base props either. This might require changes in DMS too to be handled properly. i.e. we need to differentiate between omitted and null for base properties, like we do for custom properties.
oh right - I'll see what I can in the SDK as a workaround for now 👍 |
haakonvt
force-pushed
the
fix-inst-type-not-nullable
branch
from
92f012b
to
a8b6737
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1970 +/- ##
=======================================
Coverage 90.26% 90.27%
=======================================
Files 139 139
Lines 21735 21754 +19
=======================================
+ Hits 19620 19638 +18
- Misses 2115 2116 +1
|
| @@ -87,7 +88,7 @@ def __init__( | |||
| station_360: DirectRelationReference | tuple[str, str] | None = None, | |||
| taken_at: datetime | None = None, | |||
| existing_version: int | None = None, | |||
| type: DirectRelationReference | tuple[str, str] | None = None, | |||
| type: DirectRelationReference | tuple[str, str] | None = NOT_SET, # type: ignore [assignment] | |||
There was a problem hiding this comment.
IMO, NOT_SET should be an instance of a NotSet class (or I prefer OMITTED/Omitted), and then we do this instead:
| type: DirectRelationReference | tuple[str, str] | None = NOT_SET, # type: ignore [assignment] | |
| type: DirectRelationReference | tuple[str, str] | Omitted | None = OMITTED, |
There was a problem hiding this comment.
While class Omitted with a (singleton instance) OMITTED is nicer for sure! ...I don't really like to expose this to the users (more than necessary, the default value).
It exposes internal details; users will see Omitted in the type annotation and perhaps wonder what it does, when in reality it is an implementation detail. This may lead to unnecessary confusion (do I need to know what this is/does?), which in turn may lead to unintended usage.
It also goes against the principle of what the type annotations mean; these are the allowed types you may pass. The point with the default value is when it is not passed.
Blocked until DMS starts to distinguish between "not given" and "given as null" |
https://cognitedata.atlassian.net/browse/DM-2204