Skip to content

chore(vscode): clean up format and improve logs#4409

Merged
benfdking merged 1 commit intomainfrom
fixing_is_sqlmesh_installed_call
May 15, 2025
Merged

chore(vscode): clean up format and improve logs#4409
benfdking merged 1 commit intomainfrom
fixing_is_sqlmesh_installed_call

Conversation

@benfdking
Copy link
Contributor

No description provided.

@benfdking benfdking force-pushed the fixing_is_sqlmesh_installed_call branch from 582e38d to 18cf256 Compare May 14, 2025 15:14
@benfdking benfdking requested a review from Copilot May 14, 2025 15:14
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR cleans up code formatting and logging in the VS Code extension while modernizing command execution by transitioning from synchronous to asynchronous patterns.

  • The execAsync function is converted to an async function with consolidated logging.
  • The synchronous execSync call in the format command is replaced with an asynchronous execAsync call.
  • The return type of the internalFormat function is updated to reflect that no numeric result is returned.

Reviewed Changes

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

File Description
vscode/extension/src/utilities/exec.ts Converted execAsync to async and centralized logging to improve clarity.
vscode/extension/src/commands/format.ts Replaced execSync with execAsync and updated the return type accordingly.
Comments suppressed due to low confidence (1)

vscode/extension/src/commands/format.ts:40

  • Changing the return type from Result<number, ErrorType> to Result<undefined, ErrorType> may affect consumers expecting a numeric value on success; update documentation or ensure downstream compatibility.
const internalFormat = async (): Promise<Result<undefined, ErrorType>> => {

}

export function execAsync(
export async function execAsync(
Copy link
Contributor

Choose a reason for hiding this comment

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

now that this is async, does the way AbortSignal is handled in execAsyncCore with child.kill() risk leaving behind hanging processes or it is always graceful? (I'm not fully familiar with this kind of handling, just asking to understand)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reformated to make it slightly safer 🙏

@benfdking benfdking force-pushed the fixing_is_sqlmesh_installed_call branch from 18cf256 to bb4e212 Compare May 15, 2025 09:38
@benfdking benfdking merged commit bfbacf5 into main May 15, 2025
23 checks passed
@benfdking benfdking deleted the fixing_is_sqlmesh_installed_call branch May 15, 2025 09:41
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.

3 participants