WIP: Add datapoints.delete method for improved dps delete support #1084
Conversation
erlendvollset
changed the title
erlendvollset
added
auto-update
erlendvollset
force-pushed
the
delete-dps-fix
branch
from
5002073
to
4d908cb
Compare
erlendvollset
changed the title
Codecov Report
@@ Coverage Diff @@
## master #1084 +/- ##
==========================================
+ Coverage 91.59% 91.60% +0.01%
==========================================
Files 75 75
Lines 7837 7841 +4
==========================================
+ Hits 7178 7183 +5
+ Misses 659 658 -1
|
erlendvollset
force-pushed
the
delete-dps-fix
branch
from
4d908cb
to
2fd0ad0
Compare
erlendvollset
force-pushed
the
delete-dps-fix
branch
from
4016979
to
c99f595
Compare
| for point in point_list: | ||
| for key in point: | ||
| if key not in ("id", "externalId", "timestamp"): | ||
| raise AssertionError( | ||
| f"Invalid key '{key}' in point. Must contain 'timestamp', and 'id' or 'externalId" | ||
| ) |
There was a problem hiding this comment.
I think I've seen this type of logic in multiple places in the SDK, could be split out to utils.
| raise AssertionError( | ||
| f"Invalid key '{key}' in point. Must contain 'timestamp', and 'id' or 'externalId" |
There was a problem hiding this comment.
Yep. Some fire-from-the-hip copy-pasting here 😁
| ... | ||
|
|
||
| def delete_point( | ||
| self, point: Union[Dict[str, Union[int, str, datetime]], Sequence[Dict[str, Union[int, str, datetime]]]] |
There was a problem hiding this comment.
To me it doesn't make much sense to allow str here; as these are all the "unprecise" or relative time offsets like "an hour ago" or "now".
There was a problem hiding this comment.
Agree. But this dict also contains the externslId, which has a string value.
| ... | ||
|
|
||
| def delete_point( | ||
| self, point: Union[Dict[str, Union[int, str, datetime]], Sequence[Dict[str, Union[int, str, datetime]]]] |
There was a problem hiding this comment.
...what I would like to see though was Datapoint and Datapoints supported!
There was a problem hiding this comment.
Good idea! That's probably much more useful.
| def delete_point( | ||
| self, point: Union[Dict[str, Union[int, str, datetime]], Sequence[Dict[str, Union[int, str, datetime]]]] | ||
| ) -> None: | ||
| """Delete a single datapoint from one or more time series. |
There was a problem hiding this comment.
Just a suggestion as the current sound like "delete t=200 from ts=a,b and c":
| """Delete a single datapoint from one or more time series. | |
| """Delete individual datapoints from one or more time series. |
| id = cast(Optional[int], point.get("id")) | ||
| external_id = cast(Optional[str], point.get("externalId")) | ||
| valid_range = Identifier.of_either(id, external_id).as_dict() | ||
| start = utils._time.timestamp_to_ms(point["timestamp"]) |
There was a problem hiding this comment.
May raise KeyError. Add a must-be-present check in loop above?
There was a problem hiding this comment.
The check above already ensures the existence of this key. Or did you mean smth else?
There was a problem hiding this comment.
Ah no it doesn't, it just checks that no other keys are present. I'll make a utility for doing both of these types of checks for dicts.
There was a problem hiding this comment.
Great catch!
| @@ -346,17 +395,17 @@ def delete_range( | |||
| """ | |||
| start = utils._time.timestamp_to_ms(start) | |||
| end = utils._time.timestamp_to_ms(end) | |||
| assert end > start, "end must be larger than start" | |||
| assert end >= start, "end must be larger than start" | |||
There was a problem hiding this comment.
Note to self: revert this. Having them equal doesn't make sense.
erlendvollset
changed the title
erlendvollset
changed the title
Wanna make this just datapoints.delete() instead. And that can accept timestamps and ranges, as well as Datapoint and Datapoints objects. Then we'll deprecate the other ones in V5 and remove in V6.