InstanceId TimeSeries #1825
InstanceId TimeSeries #1825
Conversation
.github/actions/setup/action.yml
Outdated
| @@ -31,6 +31,6 @@ runs: | |||
| - name: Install dependencies | |||
| shell: bash | |||
| run: | | |||
| pip install --upgrade pip poetry | |||
| python -m pip install --upgrade pip poetry | |||
There was a problem hiding this comment.
The Windows failed without this one
| @@ -87,43 +90,26 @@ def load( | |||
|
|
|||
|
|
|||
| @dataclass(frozen=True) | |||
| class InstanceId: | |||
There was a problem hiding this comment.
Moved it to avoid circular import.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1825 +/- ##
==========================================
- Coverage 92.80% 92.73% -0.07%
==========================================
Files 121 121
Lines 17871 17950 +79
==========================================
+ Hits 16585 16646 +61
- Misses 1286 1304 +18
|
cognite/client/_api/datapoints.py
Outdated
| if "instance_id" in d and isinstance(d["instance_id"], InstanceId): | ||
| d["instance_id"] = d["instance_id"].dump(include_instance_type=False) # type: ignore[assignment] | ||
| elif "instanceId" in d and isinstance(d["instanceId"], InstanceId): | ||
| d["instanceId"] = d["instanceId"].dump(include_instance_type=False) # type: ignore[assignment] |
There was a problem hiding this comment.
Why handle both instance_id and instanceId? I say just use the camelCased version. I find that it's easier for both users and developers when there's only one way to do a particular thing.
There was a problem hiding this comment.
Also seems like this logic rather should live in _verify_and_prepare_dps_objects
There was a problem hiding this comment.
Actually, it is not necessary. It is already handled in _verify_and_prepare_dps_objects 🤦
cognite/client/_api/datapoints.py
Outdated
| elif (instance_id := validated.get("instanceId")) is not None: | ||
| dps_to_insert["instanceId", (instance_id["space"], instance_id["externalId"])].extend(validated_dps) | ||
| else: | ||
| dps_to_insert["id", validated["id"]].extend(validated_dps) | ||
| return list(dps_to_insert.items()) | ||
| return [ | ||
| ((id_name, {"space": id_[0], "externalId": id_[1]} if id_name == "instanceId" else id_), data) # type: ignore[index, misc] | ||
| for (id_name, id_), data in dps_to_insert.items() | ||
| ] |
There was a problem hiding this comment.
Here you're transforming from a dict to a tuple and then back to a dict - presumably because the dict is not hashable? Might require less back-and-forth and be simpler if validate_user_input_dict_with_identifier returns Identifier objects - which should be hashable.
There was a problem hiding this comment.
Yes, you are right, simplified it. Note sure what made me go for this complex solution.
# Conflicts: # CHANGELOG.md # cognite/client/_version.py # pyproject.toml
# Conflicts: # CHANGELOG.md
erlendvollset
force-pushed
the
instance-id-timeseries
branch
3 times, most recently
from
ab65281
to
3878f0b
Compare
Improve identifier tests and fix Identifier.as_tuple()
erlendvollset
force-pushed
the
instance-id-timeseries
branch
from
3878f0b
to
8fe3752
Compare
Description
Please describe the change you have made.
Checklist:
If a new method has been added it should be referenced in cognite.rst in order to generate docs based on its docstring.