feat(DatapointsAPI): Add missing 'type' field to Datapoints[Array] data classes#2484
feat(DatapointsAPI): Add missing 'type' field to Datapoints[Array] data classes#2484haakonvt wants to merge 6 commits intopysdk-release-v8from
Conversation
Summary of ChangesHello @haakonvt, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the time series data model by introducing a new mandatory 'type' field to the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
The pull request adds the type field and enforces required fields in Datapoints and DatapointsArray. However, the change to __eq__ in Datapoints to use object identity (id()) is a critical issue that breaks value equality and will cause unit tests to fail. Additionally, the PR title is missing the ! indicator for breaking changes, and there is a type mismatch where the proto integer enum for type is passed directly to classes expecting string literals.
| def __eq__(self, other: Any) -> bool: | ||
| return ( | ||
| type(self) is type(other) | ||
| and self.id == other.id | ||
| and self.external_id == other.external_id | ||
| and list(self._get_non_empty_data_fields()) == list(other._get_non_empty_data_fields()) | ||
| ) | ||
| # Override CogniteResource __eq__ which checks exact type & dump being equal. We do not want | ||
| # this: comparing arrays with (mostly) floats is a very bad idea; also dump is exceedingly expensive. | ||
| return id(self) == id(other) |
There was a problem hiding this comment.
The __eq__ implementation has been changed to check object identity (id(self) == id(other)). This is a critical issue for DTOs as it breaks value equality; different instances with identical data will no longer be considered equal. This change also causes the test_eq unit test in this PR to fail. If dump() is too expensive, please implement a more efficient value-based equality check instead of falling back to identity.
| id: int, | ||
| is_string: bool, | ||
| is_step: bool, | ||
| type: Literal["numeric", "string", "state"], |
There was a problem hiding this comment.
Making id, is_string, is_step, and type required arguments in the constructor is a breaking change. According to the repository style guide, breaking changes must be indicated in the PR title with a ! after the type/scope.
References
- Breaking changes must be clearly indicated in the PR title with a
!after the type/scope. (link)
cognite/client/utils/_datapoints.py
Outdated
| "external_id": res.externalId, | ||
| "is_string": res.isString, | ||
| "is_step": res.isStep, | ||
| "type": res.type, |
There was a problem hiding this comment.
The res.type value from the protobuf response is an integer enum, but the Datapoints and DatapointsArray classes expect a string literal ('numeric', 'string', or 'state'). Passing the raw integer will cause type mismatches. Please map the proto enum values to the corresponding string literals.
References
- Strong Typing: Use type hints extensively with MyPy. Avoid Any when possible. (link)
21c92b9 to
b579dfb
Compare
Since we have not released v8 yet, we have a golden opportunity to implement necessary changes just-in-time for release.
Changes