Skip to content
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

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

lpereiracgn
Copy link
Contributor

@lpereiracgn lpereiracgn commented Feb 12, 2025

Description

Support for the /simulators/routines API endpoints. ( list, create and delete )

Checklist:

  • Tests added/updated.
  • Documentation updated. Documentation is generated from docstrings - these must be updated according to your change.
    If a new method has been added it should be referenced in cognite.rst in order to generate docs based on its docstring.
  • Changelog updated in CHANGELOG.md.
  • Version bumped. If triggering a new release is desired, bump the version number in _version.py and pyproject.toml per semantic versioning.

@lpereiracgn lpereiracgn requested review from a team as code owners February 12, 2025 23:21
@doctrino doctrino self-requested a review February 21, 2025 07:45
Copy link
Contributor

@doctrino doctrino left a 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:

  1. Remember to update the documentation with the new method. docs/
  2. 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.
  3. The sort object should have a more generic name, unless the plan is to never allow sorting on anything else then createdTime.

Copy link

codecov bot commented Feb 26, 2025

Codecov Report

Attention: Patch coverage is 95.95960% with 4 lines in your changes missing coverage. Please review.

Project coverage is 90.41%. Comparing base (01e65f4) to head (412335d).

Files with missing lines Patch % Lines
cognite/client/_api/simulators/routines.py 90.47% 4 Missing ⚠️
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     
Files with missing lines Coverage Δ
cognite/client/_api/simulators/__init__.py 100.00% <100.00%> (ø)
cognite/client/_api/simulators/models.py 98.30% <100.00%> (ø)
cognite/client/_api/simulators/models_revisions.py 97.91% <100.00%> (ø)
cognite/client/data_classes/simulators/__init__.py 100.00% <ø> (ø)
cognite/client/data_classes/simulators/filters.py 100.00% <100.00%> (ø)
cognite/client/data_classes/simulators/models.py 95.45% <100.00%> (+2.74%) ⬆️
cognite/client/data_classes/simulators/routines.py 100.00% <100.00%> (ø)
cognite/client/testing.py 100.00% <100.00%> (ø)
cognite/client/_api/simulators/routines.py 90.47% <90.47%> (ø)

... and 3 files with indirect coverage changes

@lpereiracgn
Copy link
Contributor Author

Good stuff. Main feedback summarized:

  1. Remember to update the documentation with the new method. docs/
  2. 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.
  3. The sort object should have a more generic name, unless the plan is to never allow sorting on anything else then createdTime.

Regarding no. 1. We will create the docs in a separate PR once we have all the endpoints in place if that's okay.

@lpereiracgn lpereiracgn requested a review from doctrino February 28, 2025 10:57
Copy link
Contributor

@doctrino doctrino left a 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!

Comment on lines +77 to +79
Returns:
Iterator[SimulatorRoutine] | Iterator[SimulatorRoutineList]: yields SimulatorRoutine one by one if chunk is not specified, else SimulatorRoutineList objects.
"""
Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

@doctrino doctrino left a 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

@doctrino doctrino requested review from a team and finnag and removed request for a team March 5, 2025 17:14
@evertoncolling evertoncolling requested review from a team and dbrattli and removed request for a team March 5, 2025 17:31
Copy link

@finnag finnag left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants