-
Notifications
You must be signed in to change notification settings - Fork 535
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
eslint(telemetry-utils): Prefix telemetry-utils before enabling no-unchecked-record-access #23428
Conversation
…hecked-record-access
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.
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
const actualValue: | ||
| TelemetryEventPropertyTypeExt | ||
| Tagged<TelemetryEventPropertyTypeExt> | ||
| undefined = actual[expectedKey]; |
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.
Woah is this really what the formatter wants? I am not a fan of how the RHS of the =
is on the same line as the end of that multi-line type. Weird, oh well.
@@ -22,7 +22,8 @@ const getMockStore = (settings: Record<string, string>): Storage => { | |||
return { | |||
getItem: (key: string): string | null => { | |||
ops.push(key); | |||
return settings[key]; | |||
// eslint-disable-next-line unicorn/no-null | |||
return settings[key] ?? null; |
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.
What's the rationale here? Is the calling code prepared to handle null
instead of undefined
?
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.
yes, getItem is typed to string | null, but returning settings[key]
results in a type of string | undefined, so I just force it to null if its undefined
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.
LGTM
eslint(telemetry-utils): Prefix telemetry-utils before enabling no-unchecked-record-access
AB#25378