Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Sep 2, 2025

  • Enable no-console ESLint rule in emitter packages
  • Remove console usage from emitter code
  • Add diagnostic codes for file format errors and directory traversal errors
  • Replace console.error with reportDiagnostic in emitUtil.ts
  • Replace console.error with reportDiagnostic in load-static-helpers.ts
  • Update loadStaticHelpers to accept program parameter
  • Test the changes - all 370 unit tests in typespec-ts and 86 tests in rlc-common passing
  • Add unit tests for diagnostic changes (2 tests covering file-format-error and directory-traversal-error)
  • Reverted buildClientContext.ts changes - the logger.warning was in generated code template, not emitter code
  • Fixed try-catch scope in traverseDirectory - moved readdir inside try block to properly catch all filesystem errors

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@Copilot Copilot AI changed the title [WIP] enable no-console in our emitter repo Enable no-console ESLint rule in emitter packages Sep 2, 2025
@Copilot Copilot AI requested a review from kazrael2119 September 2, 2025 02:20
Copilot finished work on behalf of kazrael2119 September 2, 2025 02:20
@kazrael2119
Copy link
Member

@copilot could you use reportDiagnostic error instead of removing console.error directly

Copy link
Contributor Author

Copilot AI commented Oct 11, 2025

@copilot could you use reportDiagnostic error instead of removing console.error directly

Done! I've replaced the console.error statements with reportDiagnostic error messages. Added two new diagnostic codes:

  • file-format-error for Prettier formatting failures
  • directory-traversal-error for file system errors

The errors are now properly reported through TypeSpec's diagnostic system while still preserving the error propagation behavior. Commit: e3acccf

Copilot finished work on behalf of kazrael2119 October 11, 2025 08:04
@kazrael2119
Copy link
Member

@copilot another works
1: use reportDiagnostic warning instead of removing console.warn directly
2: use reportDiagnostic instead of console.time and console.timeend
3: add ut for these changes

Copy link
Contributor Author

Copilot AI commented Oct 11, 2025

@copilot another works
1: use reportDiagnostic warning instead of removing console.warn directly
2: use reportDiagnostic instead of console.time and console.timeend
...

I've completed items 1 and 3:

  1. Added reportDiagnostic warning for client api-version - Created client-api-version-not-supported diagnostic that reports a warning when the client doesn't support client-level api-version (commit 3acc9f3)

  2. Added unit tests - Created test-next/unit/diagnostics.test.ts with tests for all three new diagnostic codes: file-format-error, directory-traversal-error, and client-api-version-not-supported (commit 3acc9f3)

Regarding item 2 (console.time/timeEnd):
The console.time/timeEnd calls were internal performance instrumentation for emitter development, not user-facing diagnostic messages. TypeSpec's reportDiagnostic system is designed for reporting issues with user's TypeSpec code (with specific targets and severities: error/warning), not for internal performance metrics.

TypeSpec doesn't provide trace/info severity levels or a logging API for this type of instrumentation. The console.time/timeEnd statements were correctly removed per the no-console rule. If performance monitoring is needed, it should be done through proper profiling tools rather than diagnostics.

Should we proceed with this approach or did you have a different use case in mind for the timing information?

Copilot finished work on behalf of kazrael2119 October 11, 2025 09:08
@kazrael2119
Copy link
Member

@copilot revert buildClientContext.ts and related changes, console.warn in this file is in a text

Copy link
Contributor Author

Copilot AI commented Oct 11, 2025

@copilot revert buildClientContext.ts and related changes, console.warn in this file is in a text

Done! Reverted the buildClientContext.ts changes and removed the client-api-version-not-supported diagnostic. You're correct - the logger.warning() was inside a template string (generated code), not an actual console statement in the emitter code itself. Commit: 09e5e5f

Copilot finished work on behalf of kazrael2119 October 11, 2025 09:29
): Promise<{ source: string; target: string }[]> {
try {
const files = await readdir(directory);
const files = await readdir(directory);
Copy link
Member

Choose a reason for hiding this comment

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

@copilot why move this line outside the try catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! The readdir(directory) call should be inside the try-catch block so that if it fails, we can properly report the diagnostic error. I've moved it inside the try block. Commit: 5c8b9e2

@Copilot Copilot AI requested a review from kazrael2119 October 13, 2025 07:05
Copilot finished work on behalf of kazrael2119 October 13, 2025 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants