feat(alpha): record view CRUD support#2722
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2722 +/- ##
==========================================
- Coverage 93.70% 93.66% -0.05%
==========================================
Files 504 504
Lines 51112 51317 +205
==========================================
+ Hits 47897 48067 +170
- Misses 3215 3250 +35
🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for record-backed views (an alpha feature) by adding the RecordViewApply class, updating the ViewsAPI to handle record views with appropriate alpha headers and warnings, and adding conversion methods like as_record_view_apply. The review feedback highlights several critical improvements: handling potential generator consumption and dictionary inputs in ViewsAPI.apply, adding validation for properties and stream count in RecordViewApply, using is_record_view consistently, and updating as_write to support polymorphic delegation for record views.
| self.name = name | ||
| self.filter = filter | ||
| self.implements: list[ViewId] = implements or [] | ||
| self.properties = properties |
There was a problem hiding this comment.
The docstring states that only mapped properties (no connections) are supported and a TypeError is raised if any other property type is passed. However, the implementation does not perform this validation. We should add a check in __init__ to enforce this constraint.
| self.properties = properties | |
| if properties: | |
| for k, v in properties.items(): | |
| if not isinstance(v, MappedPropertyApply): | |
| raise TypeError(f'Property {k!r} must be of type MappedPropertyApply, got {type(v)}') | |
| self.properties = properties |
References
- Provide meaningful error messages, for example 'got type foo, expected type bar'. (link)
There was a problem hiding this comment.
I don't think we do this usually?
There was a problem hiding this comment.
Code Review
This pull request introduces support for record-backed views (an alpha feature) in the Cognite Python SDK, adding the RecordViewApply class, updating the sync and async ViewsAPI to handle record views with appropriate alpha headers and warnings, and extending the View class with record-specific helper methods. The review feedback highlights several improvement opportunities, including resolving a potential generator exhaustion bug in ViewsAPI.apply, adding runtime validation for properties in RecordViewApply, and strengthening the robustness of record-view checks and error handling.
haakonvt
left a comment
There was a problem hiding this comment.
Mostly LGTM, a few nit:
|
|
||
| async def apply(self, view: ViewApply | Sequence[ViewApply]) -> View | ViewList: | ||
| async def apply(self, view: ViewApply | RecordViewApply | Sequence[ViewApply | RecordViewApply]) -> View | ViewList: | ||
| """`Create or update (upsert) one or more views <https://api-docs.cognite.com/20230101/tag/Views/operation/ApplyViews>`_. |
There was a problem hiding this comment.
If the record view apply has a different API doc page, lets add a link to it as well
There was a problem hiding this comment.
Do you mean the internal doc, like this? https://api-docs.cognite.com/internal/tag/Views/operation/ApplyViews
There was a problem hiding this comment.
That is the only link we have atm
There was a problem hiding this comment.
Right, it is still "internal", then let's wait 👍
Description
Added RecordViewApply for user to be able to create a record view, also a usedFor property for the /list method, so it is possible to filter record/non-record views. The output (read) type only gained an optional property "stream_id"
Checklist:
If a new method has been added it should be referenced in cognite.rst in order to generate docs based on its docstring.