-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: add guard-for-in
eslint rule
#1210
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By the way: I just fully realized only now that there are two There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. uiii, strange and bad. I checked them
and they are similar but not the same at all 🙈 It seems the one in binding-netconf seems more sophisticated. @lukesmolo? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be moved to its own issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As this one is already merged, I think we can continue in #1305 independently :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh, I missed that the PR is already merged 🙈 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -79,8 +79,9 @@ export default class NetconfCodec implements ContentCodec { | |
let nsFound = false; | ||
let aliasNs = ""; | ||
let value; | ||
for (const key in properties) { | ||
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]) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above? |
||
if (payload[key] == null) { | ||
throw new Error(`Payload is missing '${key}' field specified in TD`); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -604,11 +604,12 @@ export default class OctetstreamCodec implements ContentCodec { | |
} | ||
|
||
result = result ?? Buffer.alloc(length); | ||
for (const propertyName in schema.properties) { | ||
// TODO: Use correct type for propertySchema | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
for (const [propertyName, propertySchema] of Object.entries(schema.properties) as [string, any]) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing TODO. can't we try to use the inferred type from the removed line below? const propertySchema = schema.properties[propertyName]; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Thank you, should be fixed now :)
Unfortunately, the type of type DataSchema = { [key: string]: any; }; |
||
if (Object.hasOwnProperty.call(value, propertyName) === false) { | ||
throw new Error(`Missing property '${propertyName}'`); | ||
} | ||
const propertySchema = schema.properties[propertyName]; | ||
const propertyValue = value[propertyName]; | ||
const propertyOffset = parseInt(propertySchema["ex:bitOffset"]); | ||
const propertyLength = parseInt(propertySchema["ex:bitLength"]); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PropertyElement
doesn't work here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately,
DataSchema
is currently defined as an object whose values are typed asany
:/ So in this case, we would need to cast. Note also that these are theproperties
of theDataSchema
and not Property Affordances. In any case, the typing here needs to be revisited.