-
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
add support for the simulators endpoint #2066
base: master
Are you sure you want to change the base?
Conversation
simulators = cognite_client.simulators.list() | ||
simulator_exists = len(list(filter(lambda x: x.external_id == simulator_external_id, simulators))) > 0 | ||
if not simulator_exists: | ||
cognite_client.post( |
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 won't have a simulator.create function in the sdk
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2066 +/- ##
==========================================
+ Coverage 90.50% 90.59% +0.09%
==========================================
Files 141 145 +4
Lines 22533 22802 +269
==========================================
+ Hits 20393 20657 +264
- Misses 2140 2145 +5
|
tests/tests_integration/test_api/test_simulators/test_simulators.py
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,216 @@ | |||
# This file contains the data used to seed the test environment for the simulator tests | |||
|
|||
data_set_id = 1521375514069 |
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.
There was a comment about having this in a fixture. The reason we have it here as a common variable is that we need it as part of the resources being seeded in this file (simulator integration) and models and routines in the other pr.
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.
I don't understand this, but feel free to leave it as is - but, please add a comment on why it is necessary - or why it is just temporarily needed.
Can you change it to a data set external id? Then do lookup in a fixture? (That would make running the test suite against different CDF projects much simpler. Note also that we are going to change the test project next year, as using a staging cluster is not ideal for test flakyness 😅 )
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.
Clean and easily readable code!
Here's a few comments to get this over the finish line
List simulators: | ||
|
||
>>> from cognite.client import CogniteClient | ||
>>> client = CogniteClient() | ||
>>> res = client.simulators.list() |
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.
Are you planning to add __call__
& __iter__
? I do think almost all our APIs support the "iterative" approach.
return super().dump(camel_case=camel_case) | ||
|
||
|
||
class SimulatorIntegrationWrite(SimulatorIntegrationCore): |
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.
Why are we adding a write-class if the API only exposes filter and delete endpoints?
license_status: str | None = None, | ||
simulator_version: str | None = None, | ||
license_last_checked_time: int | None = None, | ||
connector_status: str | None = None, | ||
connector_status_updated_time: int | None = None, |
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.
Are all these valid for the write-version?
return self | ||
|
||
|
||
class SimulatorIntegration(SimulatorIntegrationCore): |
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.
Check out my comments on class Simulator
@@ -0,0 +1,216 @@ | |||
# This file contains the data used to seed the test environment for the simulator tests | |||
|
|||
data_set_id = 1521375514069 |
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.
I agree this should no be hardcoded as an internal id. it should be an external id + lookup
Description
Added the
/simulators
endpoint to the SDK. This is the first PR coming from the split of this big PR: https://github.com/cognitedata/cognite-sdk-python/pull/2006/filesAdds support for the following Resource:
Checklist:
If a new method has been added it should be referenced in cognite.rst in order to generate docs based on its docstring.