-
Notifications
You must be signed in to change notification settings - Fork 208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support completion for tspconfig.yaml file in vscode #4790
base: main
Are you sure you want to change the base?
Conversation
All changed packages have been documented.
|
You can try these changes here
|
packages/typespec-vscode/src/vscode/completion-item-provider.ts
Outdated
Show resolved
Hide resolved
@@ -98,6 +98,10 @@ async function getPackageRoot(filename: string): Promise<string | undefined> { | |||
try { | |||
const pkgPath = join(dir, "package.json"); | |||
await stat(pkgPath); | |||
const pkg = JSON.parse(await readFile(pkgPath, "utf-8")); | |||
if (!pkg.name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like a weird interpretation to decide to skip unnamed package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's because the tool would crash when the package.json found is "incomplete" because it will try to extract many information from it. I added the check/skip because I think the package should at least have a name if we are expected to get license information from it. But feel free to let me know if there are other better way to do the check.
In detail, I added the change because it crashees when I installed 'vscode-languageserver-textdocument' package which contains a package.json as below...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I guess that'd be ok, though I am not sure its needed anymore as the code was moved back in the compiler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not needed anymore. removed.
packages/typespec-vscode/src/vscode/completion-item-provider.ts
Outdated
Show resolved
Hide resolved
.vscode/launch.json
Outdated
@@ -93,6 +93,16 @@ | |||
"order": 2 | |||
} | |||
}, | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need an extra launch config for this one, can we use the js debug console in vscode to auto attach for those one off things
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
} | ||
} | ||
}, | ||
|
||
watch(path: string, onChanged: (filename: string) => void) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what kind of changelog should I add for internal change like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in a sense that change in the compiler is a breaking change as have to update all custom hosts.
With other comments talking about if we really even need this, then that change for this package would be something like Add support for typespec compiler watch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changelog added. thanks.
packages/compiler/src/core/types.ts
Outdated
* @param path path to the file/dir to watch | ||
* @param onChanged callback to be called when the file/dir changes | ||
*/ | ||
watch( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don';t think we should update the CompilerHost
here, will keep reviewing and understand why its needed but feels like the wrong place to be adding this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I don't really get why you need to add this, we already have watch funtionality in the lsp provided by vscode, we shouldn't also try to watch with our own system
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the file watch provided by vscode lsp is not suitable because the file we want to monitor may not in the workspace opened in vscode. (for example in the azure-rest-api-specs repo, i usually just open the folder of some service, but the package.json is actually at the root of the repo.). And I dont think it's a good idea to ask vscode to monitor all the folders from root recursively by default because recursive file watching is quite resource intense which should be used to minimal as suggested by vscode. So here i would prefer to add non-recursive watch explicitly only when needed.
And I added it into the CompilerHost because I think we are trying to control all the IO related operations there so that we can change the implementations and they still work with each other. thanks.
@@ -0,0 +1,213 @@ | |||
import { TextDocument } from "vscode-languageserver-textdocument"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: what do youi think about organzing this into a src/server/tspconfig/
with completion.ts
folder so if we add more functionality we can keep things nicely organized
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good to me. moved it to .../tspconfig/completion.ts
export class EmitterProvider { | ||
constructor(private npmPackageProvider: NpmPackageProvider) {} | ||
|
||
private static async isEmitter(pkg: NpmPackage) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: private methods after public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved. thanks.
private static async isEmitter(pkg: NpmPackage) { | ||
const data = await pkg.getPackageJsonData(); | ||
if (!data) return false; | ||
const dep = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feels better to just have to do the if statement twice above than extra object spread
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance we can separate the change that are not related to the tspconfig into their own pr/what needs to be reverted now that vscode extension doesn't actually have the extra features
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, reverted those changes not related and will file another PR for them. thanks.
@@ -98,6 +98,10 @@ async function getPackageRoot(filename: string): Promise<string | undefined> { | |||
try { | |||
const pkgPath = join(dir, "package.json"); | |||
await stat(pkgPath); | |||
const pkg = JSON.parse(await readFile(pkgPath, "utf-8")); | |||
if (!pkg.name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I guess that'd be ok, though I am not sure its needed anymore as the code was moved back in the compiler
defineConfig({ | ||
test: { | ||
include: ["test/unit/**/*.test.ts"], | ||
disableConsoleIntercept: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what kind of logs do you end up with? this does make it quite hard to debug sometimes if you cannot use console.log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for some reason, my console log doesnt show up without that line. Anyway, i have moved this change to another PR and will remove this line there.
import { FileService } from "./file-service.js"; | ||
import { resolveYamlScalarTarget, YamlScalarTarget } from "./yaml-resolver.js"; | ||
|
||
type ObjectJSONSchemaType = JSONSchemaType<object>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Listing issues/improvements with the completion when testing it, most of those could definitely be added as new issues and done later:
extends
could have path completion- same for
imports
I think linter
doesn't complete rule or rulesets- emitter option auto complete while inside
""
will add extra ""` - completion for known variables/parameters/env interpolation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but otherwise this is really nice, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Yeah, those likes next step for this feature. Will file issue for them when this PR is in.
related issues:
#2049, #2996