Skip to content
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

Revert #23583: Ineffective TypeScript type handling in telemetry utils #23615

Merged

Conversation

RishhiB
Copy link
Contributor

@RishhiB RishhiB commented Jan 21, 2025

The changes in #23583 for handling record access in telemetry utils didn't achieve the intended type safety improvements. While they satisfied linter rules, TypeScript still ignores undefined possibilities, leaving type analysis in these situations potentially untested.

@Copilot Copilot bot review requested due to automatic review settings January 21, 2025 19:23
@github-actions github-actions bot added the base: main PRs targeted against main branch label Jan 21, 2025
@RishhiB RishhiB changed the title Undo no unchecked record access telemetry utils revert unneeded changes from #23583 for no unchecked record access telemetry utils Jan 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (3)

packages/utils/telemetry-utils/src/test/config.spec.ts:25

  • Removing the null coalescing operator might introduce a bug if settings[key] can be undefined. Consider keeping the null coalescing operator to handle undefined values.
return settings[key];

packages/utils/telemetry-utils/src/test/errorLogging.spec.ts:692

  • [nitpick] The removal of the explicit type declaration for 'annotations' might introduce ambiguity. The type should be explicitly declared to maintain clarity.
const annotations = annotationCases[annotationCase];

packages/utils/telemetry-utils/src/mockLogger.ts:335

  • The removal of the type annotation for actualValue might introduce a bug if actual can contain values that are not of the expected type. Consider keeping the type annotation to ensure type safety.
const actualValue = actual[expectedKey];
@RishhiB RishhiB changed the title revert unneeded changes from #23583 for no unchecked record access telemetry utils revert unneeded changes from #23583 which is related to unchecked record access in telemetry utils Jan 21, 2025
@jason-ha
Copy link
Contributor

Please update the title and description - it is not that changes are "not needed". The changes don't have the desired result. They resolve the linter rule complaint, but don't change TypeScript treatment of types. TypeScript ignores the undefined possibility even when explicit. So, the analysis after that point is compromised.

@RishhiB RishhiB changed the title revert unneeded changes from #23583 which is related to unchecked record access in telemetry utils Revert #23583: Ineffective TypeScript type handling in telemetry utils Jan 21, 2025
@RishhiB RishhiB merged commit 9509975 into microsoft:main Jan 21, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants