Commands to install/uninstall roslyn-language-server for the Copilot CLI#9344
Open
phil-allen-msft wants to merge 20 commits into
Open
Commands to install/uninstall roslyn-language-server for the Copilot CLI#9344phil-allen-msft wants to merge 20 commits into
phil-allen-msft wants to merge 20 commits into
Conversation
…bove fullConfig changes to the lsp-config.json file if it exists, and write a new lsp-config.json file if it doesn't exist, assuming the .copilot folder does exist.
…uded in the vsix. Copy the file from the dotnet/skills repo now.
… on the user's machine already contains content for the roslyn-language-server. If so, make no changes.
…yn-language-server for Copilot CLI' runs multiple times, the resulting lsp-config.json has exactly the same contents as the copy of the file shipped with the product initially.
…do the same copy that you did earlier of the lsp-config.json from the dotnet/skills repo, but do it again when 'npm run ingest' is specifically run.
…ain. Change the Roslyn detection logic such that if there is a lspServer.csharp object in the read lsp-config.json, that counts as Roslyn detection = true
…epo and in the eventual extension to be 'redist'. Update all references.
… should find the lsp-config.json in the customer's HOME/.copilot directory; if it exists it should parse the file, and if the "lspServers.csharp" element exists, remove the "lspServers.csharp" element and leave all others.
…aming for the uninstall command
…or an object named 'csharp', look for an object that lists ".cs" in the "fileExtensions" collection. Use lsp-config.json in the redist folder as a guide.
…search for content to delete by looking for an lspServer object that has a "fileExtensions" with ".cs"
…t into one method and use from both existing implementation sites. Change how the callers of getCSharpServerNames works so that 'contains Roslyn Language Server' need only find one instance before eventually returns true.
…origin/main such that if they contained #sym:RoslynLanguageServer , they instead contain CSharpLsp
Fix element 1 in the above list. (create .copilot file if necessary)
… from the package.json file Now remove the "dotnet.experiments.targetPopulation" property from package.json
Update message text to reflect current behavior
…not exist, it reports an error
| expect(finalContent).toBe(packagedContent); | ||
| }); | ||
|
|
||
| test('does not modify config when any server maps .cs in fileExtensions', () => { |
Member
There was a problem hiding this comment.
Is this the behavior we want? If somebody is wanting to run us to upgrade from some other LSP to our own, don't we want to remove the other one?
Comment on lines
+20
to
+22
| await fs.mkdir(path.dirname(outputPath), { recursive: true }); | ||
| await fs.writeFile(outputPath, content, 'utf8'); | ||
| console.log(`Updated ${outputPath} from ${sourceUrl}`); |
Member
There was a problem hiding this comment.
Is there not some other npm package for downloading to a file easily?
| "createTags": "npx ts-node tasks/tags/createTags.ts", | ||
| "fixLocUrls": "npx ts-node tasks/debugger/fixLocUrls.ts", | ||
| "generateOptionsSchema": "npx ts-node tasks/debugger/generateOptionsSchema.ts", | ||
| "ingest": "npx ts-node tasks/components/ingestCopilotLspConfig.ts", |
Member
There was a problem hiding this comment.
Is this something we expect is happening automatically during a build or something we need to do on a regular basis?
| ); | ||
| context.subscriptions.push( | ||
| vscode.commands.registerCommand(installCopilotLspCommand, async () => { | ||
| const lspConfigPath = path.join(os.homedir(), '.copilot', 'lsp-config.json'); |
Member
There was a problem hiding this comment.
Rather than doing manual configuration this way, should we just be installing one of our existing plugins that contains the registration?
|
|
||
| function containsCSharpServer(lspServers: { [key: string]: unknown }): boolean { | ||
| for (const [serverName, server] of Object.entries(lspServers)) { | ||
| if (serverName === 'csharp' || serverContainsCSharpFileExtension(server)) { |
Member
There was a problem hiding this comment.
Why is this checking the sever name but the other function above is just looking for the extension?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
2 new commands (invokable from Command Palette/F1) to install and uninstall, which is really are edits on $HOME/.copilot/lsp-config.json
Reasonable error handling and notifications are present. Initial integration with microsoft-tas-client as well.
To update the 'redist' copy of lsp-config.json which is merged in as part of install, run
npm run ingestUnittests added; manually tested with no file (file created, content added), existing file which is C# (no changes), and existing file which has C++-looking bindings (content added).