chore: add `guard-for-in` eslint rule #1210

JKRhb · 2024-01-06T01:50:37Z

(Not only) in the context of #1177, I noticed that some of the for loops use the for ... in syntax instead of the for ... of syntax. Apparently, the former style is discouraged and not recommended by eslint, for example (see https://eslint.org/docs/latest/rules/guard-for-in).

This PR replaces the code lines in question and enforces the new style by activating the guard-for-in eslint rule.

codecov · 2024-01-06T01:54:20Z

Codecov Report

Attention: 65 lines in your changes are missing coverage. Please review.

Comparison is base (84327e3) 77.60% compared to head (ec67029) 76.04%.
Report is 28 commits behind head on master.

❗ Current head ec67029 differs from pull request most recent head 3e98736. Consider uploading reports for the commit 3e98736 to get more accurate results

Files	Patch %	Lines
...s/td-tools/src/util/asset-interface-description.ts	33.33%	19 Missing and 1 partial ⚠️
...kages/binding-http/src/routes/thing-description.ts	0.00%	14 Missing ⚠️
packages/binding-websockets/src/ws-server.ts	0.00%	6 Missing ⚠️
packages/core/src/servient.ts	58.33%	5 Missing ⚠️
packages/td-tools/src/td-parser.ts	83.33%	4 Missing ⚠️
packages/td-tools/src/thing-model-helpers.ts	78.94%	2 Missing and 2 partials ⚠️
...ckages/binding-netconf/src/codecs/netconf-codec.ts	0.00%	3 Missing ⚠️
packages/core/src/codecs/netconf-codec.ts	0.00%	3 Missing ⚠️
packages/core/src/exposed-thing.ts	25.00%	3 Missing ⚠️
packages/binding-coap/src/mdns-introducer.ts	0.00%	0 Missing and 2 partials ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1210      +/-   ##
==========================================
- Coverage   77.60%   76.04%   -1.57%     
==========================================
  Files          83       83              
  Lines       17311    17246      -65     
  Branches     1747     1739       -8     
==========================================
- Hits        13434    13114     -320     
- Misses       3841     4070     +229     
- Partials       36       62      +26

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

danielpeintner · 2024-01-08T09:13:41Z

Out of curiosity: Did you run into an actual problem or are you proposing the change because of the possible unexpected errors?

JKRhb · 2024-01-08T13:26:55Z

Out of curiosity: Did you run into an actual problem or are you proposing the change because of the possible unexpected errors?

I think there haven't been real problems so far, however, I think I've already read some time ago that using for of is a better approach than for in in general. So in that regard it is more like an optimization.

However, I've also noticed that there are several places where a pattern like

  for (const key in object) {
    const value = object[key];
    
    useKeyAndValue(key, value); 
  }

is being used. In terms of readability and especially type safety, I think it is better to rather use something like

  for (const [key, value] of Object.entries(object)) {
    useKeyAndValue(key, value); 
  }

instead, which is encouraged by activating the guard-for-in rule.

relu91

Some questions/required changes below.

packages/binding-coap/src/mdns-introducer.ts

relu91 · 2024-01-09T10:15:47Z

packages/binding-netconf/src/codecs/netconf-codec.ts

-                const el = properties[key];
+            // TODO: Use correct type for el
+            // eslint-disable-next-line @typescript-eslint/no-explicit-any
+            for (const [key, el] of Object.entries(properties) as [string, any]) {


PropertyElement doesn't work here?

relu91 · 2024-01-09T10:18:11Z

packages/core/src/codecs/netconf-codec.ts

-                const el = properties[key];
+            // TODO: Use correct type for el
+            // eslint-disable-next-line @typescript-eslint/no-explicit-any
+            for (const [key, el] of Object.entries(properties) as [string, any]) {


Same as above?

relu91 · 2024-01-09T10:19:47Z

packages/core/src/codecs/octetstream-codec.ts

@@ -535,11 +535,11 @@ export default class OctetstreamCodec implements ContentCodec {
        }

        result = result ?? Buffer.alloc(parseInt(parameters.length));
-        for (const propertyName in schema.properties) {
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        for (const [propertyName, propertySchema] of Object.entries(schema.properties) as [string, any]) {


Missing TODO. can't we try to use the inferred type from the removed line below?

const propertySchema = schema.properties[propertyName];

Missing TODO.

Thank you, should be fixed now :)

can't we try to use the inferred type from the removed line below?

const propertySchema = schema.properties[propertyName];

Unfortunately, the type of propertySchema was any here :/ DataSchema seems to be defined like this at the moment in the wot-typescript-definitions:

type DataSchema = { [key: string]: any; };

relu91 · 2024-01-09T10:22:25Z

packages/td-tools/src/td-parser.ts

+        property.readOnly ??= false;
+        property.writeOnly ??= false;
+        property.observable ??= false;


The new code is not exactly the same as the old one. The old one also corrected the type of the property. @danielpeintner should we keep it or the new version is better?

Good question. In theory for well-defined TDs it should not make any difference on the other hand the more complex "old" code fixes wrong types also. If it were me to decide I would stick with the old code.. but I do not have a very strong preference.

the same goes for actions and events

relu91 · 2024-01-09T10:24:06Z

packages/td-tools/src/util/asset-interface-description.ts

-                thing.properties[key].forms = [];
+                thing.properties[key] = {
+                    // @ts-expect-error Here is a strange type mismatch present
+                    forms: [],


I think it is correct, forms should contain at least one element with href defined, isn't it?

Hmm, yeah, that is true. I have now found a potential solution for achieving a type-safe implementation here. Should there be an error thrown if the forms array is empty?

See 4f2663f for an intial proposal.

relu91 · 2024-01-09T10:24:21Z

packages/td-tools/src/util/asset-interface-description.ts

+
+                thing.actions[key] = {
+                    // @ts-expect-error Here is a strange type mismatch present
+                    forms,


same comment as above.

packages/td-tools/src/util/asset-interface-description.ts

Co-authored-by: Cristiano Aguzzi <relu91@users.noreply.github.com>

egekorkan

Fine regarding the changes to mqtt package

JKRhb force-pushed the for-in-loop branch from d977d8d to 9f8e8e7 Compare January 6, 2024 20:18

chore: add guard-for-in eslint rule

2192cc9

JKRhb force-pushed the for-in-loop branch from 9f8e8e7 to 2192cc9 Compare January 6, 2024 20:24

JKRhb marked this pull request as ready for review January 8, 2024 13:16

JKRhb requested review from danielpeintner, relu91, sebastiankb and egekorkan as code owners January 8, 2024 13:16

relu91 requested changes Jan 9, 2024

View reviewed changes

JKRhb and others added 3 commits January 9, 2024 11:37

fixup! chore: add guard-for-in eslint rule

af45cd1

Co-authored-by: Cristiano Aguzzi <relu91@users.noreply.github.com>

fixup! chore: add guard-for-in eslint rule

ec67029

fixup! chore: add guard-for-in eslint rule

4f2663f

egekorkan approved these changes Jan 9, 2024

View reviewed changes

fixup! chore: add guard-for-in eslint rule

3e98736

JKRhb marked this pull request as draft February 1, 2024 09:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add `guard-for-in` eslint rule #1210

chore: add `guard-for-in` eslint rule #1210

JKRhb commented Jan 6, 2024 •

edited

codecov bot commented Jan 6, 2024 •

edited

danielpeintner commented Jan 8, 2024

JKRhb commented Jan 8, 2024

relu91 left a comment

relu91 Jan 9, 2024

relu91 Jan 9, 2024

relu91 Jan 9, 2024

JKRhb Jan 9, 2024 •

edited

relu91 Jan 9, 2024

danielpeintner Jan 11, 2024

relu91 Jan 9, 2024

JKRhb Jan 9, 2024

JKRhb Jan 9, 2024

relu91 Jan 9, 2024

egekorkan left a comment

chore: add guard-for-in eslint rule #1210

Are you sure you want to change the base?

chore: add guard-for-in eslint rule #1210

Conversation

JKRhb commented Jan 6, 2024 • edited

codecov bot commented Jan 6, 2024 • edited

Codecov Report

danielpeintner commented Jan 8, 2024

JKRhb commented Jan 8, 2024

relu91 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

JKRhb Jan 9, 2024 • edited

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

Choose a reason for hiding this comment

egekorkan left a comment

Choose a reason for hiding this comment

chore: add `guard-for-in` eslint rule #1210

chore: add `guard-for-in` eslint rule #1210

JKRhb commented Jan 6, 2024 •

edited

codecov bot commented Jan 6, 2024 •

edited

JKRhb Jan 9, 2024 •

edited