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: Patch coverage is 67.80822% with 47 lines in your changes missing coverage. Please review.

Project coverage is 76.75%. Comparing base (5e2b362) to head (ee9ebbe).

Files	Patch %	Lines
...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	90.00%	2 Missing and 3 partials ⚠️
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 ⚠️
packages/core/src/helpers.ts	80.00%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1210      +/-   ##
==========================================
+ Coverage   76.72%   76.75%   +0.02%     
==========================================
  Files          83       83              
  Lines       16085    16058      -27     
  Branches     1632     1621      -11     
==========================================
- Hits        12341    12325      -16     
+ Misses       3695     3678      -17     
- Partials       49       55       +6

☔ 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?

Unfortunately, DataSchema is currently defined as an object whose values are typed as any :/ So in this case, we would need to cast. Note also that these are the properties of the DataSchema and not Property Affordances. In any case, the typing here needs to be revisited.

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

The code should now behave the same as before and should actually only be a refactoring :)

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

egekorkan

Fine regarding the changes to mqtt package

JKRhb · 2024-07-09T20:18:27Z

Rebased this PR against the latest state of the master branch and applied some adjustments :) I think this should be ready for another review.

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

danielpeintner · 2024-07-10T08:26:22Z

packages/td-tools/src/thing-model-helpers.ts

@@ -453,15 +447,15 @@ export class ThingModelHelpers {
        return tmpThingModels;
    }

-    private static getThingModelRef(data: Record<string, unknown>): Record<string, unknown> {
-        const refs = {} as Record<string, unknown>;
+    private static getThingModelRef(data: Record<string, unknown>): Record<string, string> {


A general comment. Changes to ThingModel code should also go to https://github.com/eclipse-thingweb/td-tools/tree/main/node/thing-model later...

I created eclipse-thingweb/td-tools#33

danielpeintner

I looked over the changes and the they seem fine now. Thanks!

JKRhb · 2024-07-11T06:27:36Z

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

By the way: I just fully realized only now that there are two netconf-codec.ts files, one in the core package and one in the binding-netconf package. I wonder if we should remove one of them (probably the one in the core package)?

uiii, strange and bad. I checked them

https://github.com/eclipse-thingweb/node-wot/blob/master/packages/binding-netconf/src/codecs/netconf-codec.ts
vs.

https://github.com/eclipse-thingweb/node-wot/blob/master/packages/core/src/codecs/netconf-codec.ts

and they are similar but not the same at all 🙈

It seems the one in binding-netconf seems more sophisticated. @lukesmolo?

I checked them

* https://github.com/eclipse-thingweb/node-wot/blob/master/packages/binding-netconf/src/codecs/netconf-codec.ts vs. * https://github.com/eclipse-thingweb/node-wot/blob/master/packages/core/src/codecs/netconf-codec.ts

and they are similar but not the same at all 🙈

Hmm, yeah, if I see it correctly, commit 56797cd intended to move the codec to the binding directory but also left it in the core package 😅

I think this should be moved to its own issue.

I created #1304.
@JKRhb shall we fix it independently and you merge in master in this PR? Would that be okay for you?

As this one is already merged, I think we can continue in #1305 independently :)

#1305 has been merged. @JKRhb maybe you can merge in master and we can continue with merging this PR ...

Ahh, I missed that the PR is already merged 🙈

JKRhb · 2024-07-11T06:38:49Z

I looked over the changes and the they seem fine now. Thanks!

Thank you! :) The PR would now be ready to be merged – however, I guess we should wait for #1302 et al. to resolve the issue with the eslint step.

danielpeintner · 2024-07-11T12:39:27Z

@relu91 are you okay with these changes?

JKRhb force-pushed the for-in-loop branch 2 times, most recently 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

egekorkan approved these changes Jan 9, 2024

View reviewed changes

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

JKRhb force-pushed the for-in-loop branch from 3e98736 to 1a0ec7e Compare July 9, 2024 19:51

JKRhb marked this pull request as ready for review July 9, 2024 20:18

JKRhb force-pushed the for-in-loop branch from 18b2ddc to 30d43b8 Compare July 9, 2024 20:23

danielpeintner reviewed Jul 10, 2024

View reviewed changes

packages/td-tools/src/td-parser.ts Outdated Show resolved Hide resolved

danielpeintner reviewed Jul 10, 2024

View reviewed changes

danielpeintner approved these changes Jul 10, 2024

View reviewed changes

JKRhb commented Jul 11, 2024

View reviewed changes

JKRhb force-pushed the for-in-loop branch from ee9ebbe to a082bf7 Compare July 11, 2024 06:36

chore: add guard-for-in eslint rule

8e172ad

JKRhb force-pushed the for-in-loop branch from a082bf7 to 8e172ad Compare July 11, 2024 12:19

relu91 approved these changes Jul 11, 2024

View reviewed changes

relu91 merged commit 4500eef into eclipse-thingweb:master Jul 11, 2024
13 checks passed

JKRhb deleted the for-in-loop branch July 11, 2024 14:24

danielpeintner mentioned this pull request Jul 12, 2024

two netconf-codec.ts files #1304

Closed

danielpeintner mentioned this pull request Jul 12, 2024

Take over ThingModel changes from node-wot PR 1210 eclipse-thingweb/td-tools#33

Closed

danielpeintner added a commit to danielpeintner/td-tools that referenced this pull request Jul 12, 2024

refactor: take over changes from eclipse-thingweb/node-wot#1210

612d7db

danielpeintner mentioned this pull request Jul 12, 2024

refactor: take over changes from node-wot PR 1210 eclipse-thingweb/td-tools#34

Merged

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

Loading

codecov bot commented Jan 6, 2024 •

edited

Loading

danielpeintner commented Jan 8, 2024

JKRhb commented Jan 8, 2024

relu91 left a comment

relu91 Jan 9, 2024

JKRhb Jul 9, 2024

relu91 Jan 9, 2024

relu91 Jan 9, 2024

JKRhb Jan 9, 2024 •

edited

Loading

relu91 Jan 9, 2024

danielpeintner Jan 11, 2024

JKRhb Jul 9, 2024

egekorkan left a comment

JKRhb commented Jul 9, 2024

danielpeintner Jul 10, 2024

danielpeintner Jul 12, 2024

danielpeintner left a comment

JKRhb Jul 11, 2024 •

edited

Loading

danielpeintner Jul 11, 2024

JKRhb Jul 11, 2024

relu91 Jul 11, 2024

danielpeintner Jul 12, 2024

JKRhb Jul 12, 2024

danielpeintner Jul 12, 2024 •

edited

Loading

danielpeintner Jul 12, 2024 •

edited

Loading

JKRhb commented Jul 11, 2024

danielpeintner commented Jul 11, 2024

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

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

Conversation

JKRhb commented Jan 6, 2024 • edited Loading

codecov bot commented Jan 6, 2024 • edited Loading

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

Choose a reason for hiding this comment

JKRhb Jan 9, 2024 • 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

egekorkan left a comment

Choose a reason for hiding this comment

JKRhb commented Jul 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielpeintner left a comment

Choose a reason for hiding this comment

JKRhb Jul 11, 2024 • 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

danielpeintner Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

danielpeintner Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

JKRhb commented Jul 11, 2024

danielpeintner commented Jul 11, 2024

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

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

JKRhb commented Jan 6, 2024 •

edited

Loading

codecov bot commented Jan 6, 2024 •

edited

Loading

JKRhb Jan 9, 2024 •

edited

Loading

JKRhb Jul 11, 2024 •

edited

Loading

danielpeintner Jul 12, 2024 •

edited

Loading

danielpeintner Jul 12, 2024 •

edited

Loading