Angular migration#10019
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the migration tool by introducing support for Angular projects. It enables the tool to detect the project's framework and dynamically generate appropriate VS Code debug configurations, specifically adding a new configuration tailored for Angular applications while maintaining existing support for Next.js. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for migrating Angular projects from Firebase Studio by detecting the framework and generating the appropriate VS Code launch configuration. The changes are well-structured, but there are opportunities to improve code quality by adhering more strictly to the style guide (avoiding any) and reducing code duplication in both the implementation and the tests. I've provided specific suggestions to address these points.
| ).to.be.true; | ||
| }); | ||
|
|
||
| it("should perform a full migration for Angular successfully", async () => { |
There was a problem hiding this comment.
This test case is very similar to the 'Next.js' test case above. To improve maintainability and reduce code duplication, consider extracting the common mocking logic into a setup function that can be called by both tests. The setup function could take the framework as an argument to handle framework-specific stubs.
References
- The style guide recommends using helper functions to encapsulate branching and reduce nesting. This principle applies to reducing duplication in tests for better maintainability. (link)
There was a problem hiding this comment.
Agreed - some of this could definitely be moved into a beforeEach or a helper function. Not blocking tho.
There was a problem hiding this comment.
Fixed the copy-pasta, much better now
| sandbox.stub(prompt, "confirm").resolves(false); | ||
|
|
||
| // Mock framework discovery as Angular | ||
| sandbox.stub(frameworks, "discover").resolves({ framework: "angular" } as any); |
There was a problem hiding this comment.
Using as any is an escape hatch that should be avoided according to the repository's style guide. The discover function returns a Discovery object. Please provide an object that matches the expected type to avoid using as any.
| sandbox.stub(frameworks, "discover").resolves({ framework: "angular" } as any); | |
| sandbox.stub(frameworks, "discover").resolves({ framework: "angular", mayWantBackend: true }); |
References
- The style guide prohibits using
anyas an escape hatch. Proper types or interfaces should be used instead. (link)
| logger.info("✅ Updated .vscode/settings.json with startup preferences"); | ||
|
|
||
| const launchJson = { | ||
| const launchJson: any = { |
There was a problem hiding this comment.
Using any is an escape hatch that should be avoided according to the repository's style guide. Please define a proper type for launchJson. You could define an interface for the VS Code launch configuration structure, for example:
interface LaunchConfiguration {
type: string;
request: string;
name: string;
runtimeExecutable: string;
runtimeArgs: string[];
port: number;
console: string;
preLaunchTask: string;
}
interface LaunchJson {
version: string;
configurations: LaunchConfiguration[];
}References
- The style guide prohibits using
anyas an escape hatch. Proper types or interfaces should be used instead. (link)
| if (framework === "angular") { | ||
| launchJson.configurations.push({ | ||
| type: "node", | ||
| request: "launch", | ||
| name: "Angular: debug server-side", | ||
| runtimeExecutable: "npm", | ||
| runtimeArgs: ["run", "start"], | ||
| port: 9002, | ||
| console: "integratedTerminal", | ||
| preLaunchTask: "npm-install", | ||
| }); | ||
| } else { | ||
| // Default to Next.js (or "next" in discovery) | ||
| launchJson.configurations.push({ | ||
| type: "node", | ||
| request: "launch", | ||
| name: "Next.js: debug server-side", | ||
| runtimeExecutable: "npm", | ||
| runtimeArgs: ["run", "dev"], | ||
| port: 9002, | ||
| console: "integratedTerminal", | ||
| preLaunchTask: "npm-install", | ||
| }); | ||
| } |
There was a problem hiding this comment.
The launch configurations for Angular and Next.js are very similar. To reduce code duplication and improve maintainability, you can extract the common properties into a base configuration object and then spread it into the framework-specific configurations.
const baseConfiguration = {
type: "node",
request: "launch",
runtimeExecutable: "npm",
port: 9002,
console: "integratedTerminal",
preLaunchTask: "npm-install",
};
if (framework === "angular") {
launchJson.configurations.push({
...baseConfiguration,
name: "Angular: debug server-side",
runtimeArgs: ["run", "start"],
});
} else {
// Default to Next.js (or "next" in discovery)
launchJson.configurations.push({
...baseConfiguration,
name: "Next.js: debug server-side",
runtimeArgs: ["run", "dev"],
});
}References
- The style guide recommends reducing nesting and considering helper functions to encapsulate branching. This principle can be applied here to reduce duplication by creating a base configuration. (link)
There was a problem hiding this comment.
Verbose is good
joehan
left a comment
joehan
left a comment
There was a problem hiding this comment.
LGTM, tho the ai suggestion are good here
| ).to.be.true; | ||
| }); | ||
|
|
||
| it("should perform a full migration for Angular successfully", async () => { |
There was a problem hiding this comment.
Agreed - some of this could definitely be moved into a beforeEach or a helper function. Not blocking tho.
| logger.info("✅ Updated .vscode/settings.json with startup preferences"); | ||
|
|
||
| const launchJson = { | ||
| const launchJson: any = { |
3 participants
Angular migration. This one is a little verbose but it's mostly the launch.json bit we want to capture here.