[DOGE-106] Instance aggregate/search limit=None means maximum #2121
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2121 +/- ##
=======================================
Coverage 90.38% 90.38%
=======================================
Files 152 152
Lines 23127 23131 +4
=======================================
+ Hits 20903 20908 +5
+ Misses 2224 2223 -1
🚀 New features to boost your workflow:
|
doctrino
requested review from
a team and
finnag
and removed request for
a team
| body: dict[str, Any] = { | ||
| "view": view.dump(camel_case=True), | ||
| "instanceType": instance_type, | ||
| "limit": self._AGGREGATE_LIMIT if is_unlimited(limit) or limit is None else limit, |
There was a problem hiding this comment.
| "limit": self._AGGREGATE_LIMIT if is_unlimited(limit) or limit is None else limit, | |
| "limit": self._AGGREGATE_LIMIT if is_unlimited(limit) else limit, |
ref its implementation:
def is_unlimited(limit: float | int | None) -> bool:
return limit in {None, -1, math.inf}There was a problem hiding this comment.
The limit is None is just for mypy.
There was a problem hiding this comment.
My mypy is happy without it
There was a problem hiding this comment.
Ok then it is me that did not trust mypy
|
Perhaps change |
There was a problem hiding this comment.
Previous comment was sent prematurely. Trying again:
How do you know the behavior is as you write? There should be tests.
What happens if you specify -2, 0, or 1040 as limit?
Have we publicly documented 3 different ways of getting the max limit (-1, None, inf) ? If we have, we'll have to support that more or less forever. If we haven't, do we want to support it forever?
Do we really support floats as limit (math.inf is a float..)
The the API will give a 400 and tell you that it is an invalid limit. The philosophy of the PySDK is that we do as little validation of the input to the API as possible, this is such that we do not have to keep up with the validation of the API side.
We support that for all other limit parameters in the SDK, so we have already committed to supporting it. I think we should continue to do so here, so we get consistency.
Yes, again for consistency with all other limit parameters. |
doctrino
changed the title
| @@ -164,6 +164,11 @@ def _load(data: dict, cognite_client: CogniteClient | None = None) -> NodeApply | |||
| class InstancesAPI(APIClient): | |||
| _RESOURCE_PATH = "/models/instances" | |||
|
|
|||
| def __init__(self, config: ClientConfig, api_version: str | None, cognite_client: CogniteClient) -> None: | |||
| super().__init__(config, api_version, cognite_client) | |||
| self._AGGREGATE_LIMIT = 1000 | |||
There was a problem hiding this comment.
Shouldn't these be class variables instead?
There was a problem hiding this comment.
Hmm, that could be. Consistently in the SDK we have limits as instance
attributes. Suggest that if we move these two class variables, we do it everywhere, which should in that case be a separate PR.
|
Even if you don't do validation in the SDK, you still have to test that feature you make here actually works, that it's possible to return maximum by using e.g |
doctrino
requested review from
finnag,
dbrattli,
a team and
larshagencognite
and removed request for
a team
Description
Fixed
client.data_modeling.instances.aggregate()method now correctly returns maximum, 1000, results when settingthe
limitparameter toNone,-1, ormath.inf.Checklist:
If a new method has been added it should be referenced in cognite.rst in order to generate docs based on its docstring.