-
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 simulator routines #2114
base: master
Are you sure you want to change the base?
Conversation
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.
Good stuff. Main feedback summarized:
- Remember to update the documentation with the new method. docs/
- Don't use the
SumulatorRoutinesFilter
instead use parameters directly in the methods. The reason is that forcing the user to do an import is cumbersome. - The sort object should have a more generic name, unless the plan is to never allow sorting on anything else then createdTime.
tests/tests_integration/test_api/test_simulators/test_routines.py
Outdated
Show resolved
Hide resolved
tests/tests_integration/test_api/test_simulators/test_routines.py
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2114 +/- ##
==========================================
+ Coverage 90.38% 90.41% +0.02%
==========================================
Files 152 154 +2
Lines 23127 23214 +87
==========================================
+ Hits 20903 20988 +85
- Misses 2224 2226 +2
|
Regarding no. 1. We will create the docs in a separate PR once we have all the endpoints in place if that's okay. |
tests/tests_integration/test_api/test_simulators/test_routines.py
Outdated
Show resolved
Hide resolved
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.
Solid work. I am happy :)
Just a few docstrings and trigger the last warning and you are good to go!
Returns: | ||
Iterator[SimulatorRoutine] | Iterator[SimulatorRoutineList]: yields SimulatorRoutine one by one if chunk is not specified, else SimulatorRoutineList objects. | ||
""" |
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.
Can you add a few examples? See similar methods in other parts of the API
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 added an example in the list
method, should I also include here in the __call__
method?
We have not done that in the other endpoints
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.
All good from me
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.
NT - leaving this comment here to remove this PR from my risk review dashboard. Dag is doing the risk review.
Description
Support for the
/simulators/routines
API endpoints. ( list, create and delete )Checklist:
If a new method has been added it should be referenced in cognite.rst in order to generate docs based on its docstring.