Skip to content

Conversation

@EliseWei
Copy link
Owner

@EliseWei EliseWei commented May 18, 2023

We've changed our code convention to include semicolons, so I've added them in four places.

Copy link
Owner Author

@EliseWei EliseWei left a comment

Choose a reason for hiding this comment

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

I think there are some concerns with this code, generally. Not the semicolons, those are fine.

];

const installExtensions = async (includeTracking) => {
console.group('Installing extensions...')
Copy link
Owner Author

Choose a reason for hiding this comment

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

This should be console.log

@@ -4,13 +4,13 @@ const extensions = [
'SpeedyLom.dislexic-vscode',
'streetsidesoftware.code-spell-checker',
'vscode-icons-team.vscode-icons'
Copy link
Owner Author

Choose a reason for hiding this comment

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

Need a comma after every item

}))
console.log('✅ Extensions installed')
console.groupEnd()
}
Copy link
Owner Author

Choose a reason for hiding this comment

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

Missing whitespace.

@EliseWei
Copy link
Owner Author

@CodiumAI-Agent /improve

@QodoAI-Agent
Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Impact
Possible issue
Guard console grouping and outcomes

Use console.groupEnd() even when installation fails to avoid leaving the console in
a grouped state. Ensure the success message only prints after all installs complete
and handle errors with a clear summary flag.

src/extensions.js [15-30]

 const installExtensions = async (includeTracking) => {
   console.group('Installing extensions...')
   const extToInstall = includeTracking ? [...extensions, ...trackingExtensions] : extensions
 
-
-  await Promise.all(extToInstall.map(async (extension) => {
+  let hadErrors = false;
+  try {
+    await Promise.all(extToInstall.map(async (extension) => {
       exec(`code --install-extension ${extension}`, (error, stdout) => {
         if (error) {
-          console.log(error)
+          hadErrors = true;
+          console.log(error);
           console.log(`⚠️ Failed to install ${extension}`);
         }
-      })
-  }))
-  console.log('✅ Extensions installed')
-  console.groupEnd()
+      });
+    }));
+    console.log(hadErrors ? '⚠️ Completed with some failures' : '✅ Extensions installed');
+  } finally {
+    console.groupEnd();
+  }
 };
Suggestion importance[1-10]: 6

__

Why: The suggestion is contextually valid: it ensures console.groupEnd() always runs and adds a summary flag, improving robustness and user feedback. However, its impact is moderate and it doesn't address the underlying async pattern with exec callbacks not truly awaited.

Low

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