Generate Code from TypeSpec in VSCode #5312
Conversation
chunyu3
requested review from
RodgeFu,
bterlson,
markcowl,
allenjzhang and
timotheeguerin
as code owners
microsoft-github-policy-service
bot
added
the
ide
|
All changed packages have been documented.
|
| ); | ||
| return; | ||
| } | ||
| const outputDir = path.resolve(baseDir, selectedEmitter.emitterKind, selectedEmitter.language); |
There was a problem hiding this comment.
path.resolve? shouldn't be joinPath?
| devDependencies = "devDependencies", | ||
| } | ||
|
|
||
| export interface NpmPackageInfo { |
There was a problem hiding this comment.
there is a NodePackage type in compiler you can use directly
There was a problem hiding this comment.
VSCode Extension may not library dependent on @typespec/compiler. So we may not import type from compiler.
| import logger from "./log/logger.js"; | ||
| import { ExecOutput, loadModule, spawnExecution, spawnExecutionEvents } from "./utils.js"; | ||
|
|
||
| export enum InstallationAction { |
There was a problem hiding this comment.
usually we prefer to use union to enum. like: InstallationAction = "install" | "upgrade" | "skip" | "cancel". same for below
There was a problem hiding this comment.
I want to use the enum to define the supported action, user don't need to pass in the value. Union defined is to define an type or string literal, it may not well control/guide.
| .split("\n") | ||
| .forEach((line) => logger.info(line)); | ||
| }; | ||
| export const onStdioError = (str: string) => { |
There was a problem hiding this comment.
People can't figure out what these two methods for just by name which looks more like an event name... Why not just use the spawnExecutionAndLogToOutput which would log the stdio/stderr which is just what you want to do here, isn't it?
There was a problem hiding this comment.
spawnExecutionAndLogToOutput will log out all the logs in one string. I want to split it one line by one line which will be more clear.
| ); | ||
| }; | ||
|
|
||
| export async function getEntrypointTspFile(tspPath: string): Promise<string | undefined> { |
There was a problem hiding this comment.
i think you need to search parent folder for the main.tsp too. you can refer to getMainFileForDocument in core\server\compiler-service.ts
There was a problem hiding this comment.
I don't think we need to search parent folder. This function is to retrieve out all the tsp project (main.tsp) under a specified folder.
| try { | ||
| const files = await readdir(baseDir); | ||
| if (files && files.length === 1 && files[0].endsWith(".tsp")) { | ||
| return path.resolve(baseDir, files[0]); |
There was a problem hiding this comment.
it's better to report error than pick a random file which wouldn't right in most case
There was a problem hiding this comment.
In this case, there only one file, and no main.tsp, it can be the entrypoint file.
| import vscode from "vscode"; | ||
| import { EmitterKind } from "./emitter.js"; | ||
|
|
||
| export interface EmitQuickPickItem extends vscode.QuickPickItem { |
There was a problem hiding this comment.
feels you dont need to define this type explicitly either.
There was a problem hiding this comment.
The EmitQuickPickItem has several extra properties, it is not simple one, so I think we can define an explicit type here to clear understanding and good readability in future maintenance.
| const dependenciesToInstall = await npmUtil.ensureNpmPackageDependencyToUpgrade( | ||
| selectedEmitter.package, | ||
| version, | ||
| npmDependencyType.peerDependencies, |
There was a problem hiding this comment.
dont we need to check dep and devdep?
There was a problem hiding this comment.
Current all the emitters has peer dependency, current we only need check peerDependencies. In future, when we support customized emitter which may not follow our rule to define peerDependencies, we will add more dependency check.
|
You can try these changes here
|
Fix #5025
Integrate Code Generation in VSCode extension