Minor cleanups of some python antipatterns #1386

HaydenCognite · 2023-09-29T09:27:35Z

Description

Take on the range/len cleanup described here. Took the liberty of cleaning up a few other things like some f-strings, list comprehensions, etc.

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.

haakonvt

I know this is still in draft so maybe you have already addressed some of these comments; but I would just like to give some feedback after the workshop session 😄

Biggest remark: you can not remove the initial zero in range when used with three arguments.

haakonvt · 2023-10-02T20:48:45Z

cognite/client/_api/assets.py

        chunk_size = 100
-        for i in range(0, len(ids), chunk_size):
-            tasks.append({"parent_ids": ids[i : i + chunk_size], "limit": -1})
+        tasks = [{"parent_ids": ids[i : i + chunk_size], "limit": -1} for i in range(len(ids), chunk_size)]


Please instead use split_into_chunks from utils.

cognite/client/_api/datapoints.py

tests/tests_unit/test_api/test_sequences.py

cognite/client/_api/assets.py

haakonvt · 2023-10-03T08:28:53Z

tests/tests_integration/test_api/test_geospatial.py

@@ -277,12 +276,12 @@ def test_update_multiple_features(self, cognite_client, allow_crs_transformation
            feature_type_external_id=test_feature_type.external_id,
            feature=[
                Feature(external_id=test_features[idx].external_id, temperature=6.237, pressure=12.21, volume=34.43)
-                for idx in range(0, len(test_features))
+                for idx in range(len(test_features))


Look at how can you remove range(len

haakonvt · 2023-10-03T08:29:25Z

tests/tests_integration/test_api/test_geospatial.py

            ],
            allow_crs_transformation=allow_crs_transformation,
            chunk_size=2,
        )
-        for idx in range(0, len(test_features)):
+        for idx in range(len(test_features)):


Same here, try to remove the range-len-construct

haakonvt · 2023-10-03T08:32:00Z

tests/tests_integration/test_api/test_templates.py

+    len_new_view = 1000
+    events = [
+        Event(
+            external_id=f"test_evt_templates_1_{i}",
+            type="test_templates_1",
+            start_time=i * len_new_view,
+        )
+        for i in range(len_new_view + 1)
+    ]


Suggested change

len_new_view = 1000

events = [

Event(

external_id=f"test_evt_templates_1_{i}",

type="test_templates_1",

start_time=i * len_new_view,

)

for i in range(len_new_view + 1)

]

events = [

Event(external_id=f"test_evt_templates_1_{i}", type="test_templates_1", start_time=i * 1000)

for i in range(1001)

]

This won't work because now len_new_view is undefined.

I promise I don't just give variable names to all random variables/arguments, only when they are repeated. 😇

Just use a literal, that was my point 😄

Yeah but with mine you only have one place to update it in case it needs to be changed.

But they are not the same variable! First says do 1000 loops, the other says change start time to be seconds, not ms

Ah whoops! Misread it then. Good thing I already made the change though.

tests/tests_unit/test_api/test_sequences.py

cognite/client/_api/assets.py

haakonvt · 2023-10-04T09:45:04Z

cognite/client/_api/datapoints.py

-        tasks = []
-        for dps_object_list in dps_object_lists:
-            tasks.append((dps_object_list,))
+        tasks = [(dps_object_list,) for dps_object_list in dps_object_lists]


Looks like list(zip(dps_object_lists)) would work 😜 Not that I think you should change it.

I could go for either or, honestly. The reader knows that these are a kind of "type/variable/data massaging". 🤷

haakonvt · 2023-10-04T10:09:14Z

cognite/client/_api/synthetic_time_series.py

+        for exp in expressions_to_iterate:
+            expression, short_expression = self._build_expression(exp, variables, aggregate, granularity)


Nice, this is exactly what I had in mind

codecov · 2023-10-04T10:12:04Z

Codecov Report

Merging #1386 (1ef99b6) into master (44f11db) will decrease coverage by 0.18%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1386      +/-   ##
==========================================
- Coverage   90.65%   90.48%   -0.18%     
==========================================
  Files         117      117              
  Lines       14261    14255       -6     
==========================================
- Hits        12929    12898      -31     
- Misses       1332     1357      +25

Files	Coverage Δ
cognite/client/_api/assets.py	`95.16% <100.00%> (-0.07%)`	⬇️
cognite/client/_api/datapoints.py	`96.28% <100.00%> (-0.02%)`	⬇️
cognite/client/_api/synthetic_time_series.py	`96.05% <100.00%> (ø)`

... and 7 files with indirect coverage changes

haakonvt · 2023-10-04T12:19:45Z

Btw, please update the PR title 😅

HaydenCognite · 2023-10-04T12:41:30Z

Btw, please update the PR title 😅

Aww... 😢 😛

haakonvt · 2023-10-05T13:15:18Z

tests/tests_integration/test_api/test_templates.py

-    events = []
-    for i in range(0, 1001):
-        events.append(Event(external_id="test_evt_templates_1_" + str(i), type="test_templates_1", start_time=i * 1000))
+    events = [


One more idea, make this session scoped to just run once

haakonvt

One last comment (unrelated to your changes, just made me aware of a bad fixture)

fixed string formatting Co-authored-by: Håkon V. Treider <[email protected]>

haakonvt requested changes Oct 2, 2023

View reviewed changes

haakonvt reviewed Oct 3, 2023

View reviewed changes

cognite/client/_api/assets.py Outdated Show resolved Hide resolved

haakonvt reviewed Oct 3, 2023

View reviewed changes

tests/tests_unit/test_api/test_sequences.py Outdated Show resolved Hide resolved

HaydenCognite marked this pull request as ready for review October 4, 2023 07:07

HaydenCognite requested review from a team as code owners October 4, 2023 07:07

haakonvt reviewed Oct 4, 2023

View reviewed changes

cognite/client/_api/assets.py Outdated Show resolved Hide resolved

haakonvt reviewed Oct 4, 2023

View reviewed changes

HaydenCognite changed the title ~~Doge26~~ Minor cleanups of some python antipatterns Oct 4, 2023

haakonvt reviewed Oct 5, 2023

View reviewed changes

haakonvt approved these changes Oct 5, 2023

View reviewed changes

Hayden Hohns and others added 11 commits October 5, 2023 20:30

added some fixes to ranges and to f-strings

4fd198c

found a few other things to clean up

c277420

Update tests/tests_unit/test_api/test_sequences.py

84e744b

fixed string formatting Co-authored-by: Håkon V. Treider <[email protected]>

fixed feedback regarding range function and some readability issues

4e57a15

fixed line to use split_into_chunks (thanks Håkon)

4e01460

minor fixes to test_geospatial to remove range(len( examples

1ff8d57

think I fixed bug that lead to hammering greenfield

0f7d172

found another range(len( I could fix

b5774f0

updated with literals

3b2ca5f

cleaned up fixture to run only once

042f5d0

don't fail at creating 1k events over-and-over...

1ef99b6

haakonvt force-pushed the doge26 branch from bea4dd0 to 1ef99b6 Compare October 5, 2023 18:30

haakonvt enabled auto-merge (squash) October 5, 2023 18:31

haakonvt merged commit 4cd59a1 into master Oct 5, 2023
7 checks passed

haakonvt deleted the doge26 branch October 5, 2023 18:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor cleanups of some python antipatterns #1386

Minor cleanups of some python antipatterns #1386

HaydenCognite commented Sep 29, 2023

haakonvt left a comment

haakonvt Oct 2, 2023

haakonvt Oct 3, 2023

haakonvt Oct 3, 2023

haakonvt Oct 3, 2023 •

edited

Loading

HaydenCognite Oct 4, 2023

HaydenCognite Oct 4, 2023 •

edited

Loading

haakonvt Oct 4, 2023

HaydenCognite Oct 4, 2023

haakonvt Oct 4, 2023

HaydenCognite Oct 4, 2023

haakonvt Oct 4, 2023

HaydenCognite Oct 4, 2023 •

edited

Loading

haakonvt Oct 4, 2023

codecov bot commented Oct 4, 2023 •

edited

Loading

haakonvt commented Oct 4, 2023

HaydenCognite commented Oct 4, 2023

haakonvt Oct 5, 2023

haakonvt left a comment

		for exp in expressions_to_iterate:
		expression, short_expression = self._build_expression(exp, variables, aggregate, granularity)

Minor cleanups of some python antipatterns #1386

Minor cleanups of some python antipatterns #1386

Conversation

HaydenCognite commented Sep 29, 2023

Description

Checklist:

haakonvt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haakonvt Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaydenCognite Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaydenCognite Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 4, 2023 • edited Loading

Codecov Report

haakonvt commented Oct 4, 2023

HaydenCognite commented Oct 4, 2023

Choose a reason for hiding this comment

haakonvt left a comment

Choose a reason for hiding this comment

haakonvt Oct 3, 2023 •

edited

Loading

HaydenCognite Oct 4, 2023 •

edited

Loading

HaydenCognite Oct 4, 2023 •

edited

Loading

codecov bot commented Oct 4, 2023 •

edited

Loading