-
Notifications
You must be signed in to change notification settings - Fork 676
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
Tell the language server to open projects if no solution exists #6062
Tell the language server to open projects if no solution exists #6062
Conversation
const projectUris = await vscode.workspace.findFiles( | ||
'**/*.csproj', | ||
'**/node_modules/**', | ||
options.omnisharpOptions.maxProjectResults |
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 is an omnisharp option that I'm consuming here even on the non-omnisharp side, in case there is a user out there where this limit is significant. Having this not be a dotnet option is a bit of a violation of naming though, but I'm not sure I want to create a new option and setup the migration at this moment until we have a chance to spec it a bit more. Put another way, I'm OK with this bug at the moment.
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.
Fine with as-is - lets have an issue to track. But it should be pretty straightforward to migrate the option name if you wanted. We just put the O# name as the fallback, then add a value to https://github.com/dotnet/vscode-csharp/blob/main/src/shared/migrateOptions.ts
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.
To echo something I mentioned to @dibarbet privately: making the fallback work is easy, but the option is a bit funky in this case since the filtering isn't exactly deterministic in what it will filter out: it prevents certain large repositories from causing pathological problems, but it's not ideal.
This is the server side portion of dotnet/vscode-csharp#6062
Signing off on commit 2. |
@@ -114,6 +115,9 @@ export class RoslynLanguageServer { | |||
*/ | |||
private _solutionFile: vscode.Uri | undefined; | |||
|
|||
/** The project files previously opened; we hold onto this for the same reason as _solutionFile. */ | |||
private _projectFiles: vscode.Uri[] = new Array<vscode.Uri>(); |
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 understand the solution file is also a moving target to cache. But the contents were cache agnostic. The project file list on the other hand as a cache seems more volatile if cache and not tracked continuously.
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.
Not a blocker, just want to get on the same page
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This is the server side portion of dotnet/vscode-csharp#6062
const projectUris = await vscode.workspace.findFiles( | ||
'**/*.csproj', | ||
'**/node_modules/**', | ||
options.omnisharpOptions.maxProjectResults |
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.
Fine with as-is - lets have an issue to track. But it should be pretty straightforward to migrate the option name if you wanted. We just put the O# name as the fallback, then add a value to https://github.com/dotnet/vscode-csharp/blob/main/src/shared/migrateOptions.ts
) { | ||
const protocolUri = this._languageClient.clientOptions.uriConverters!.code2Protocol(this._solutionFile); | ||
await this._languageClient.sendNotification('solution/open', new OpenSolutionParams(protocolUri)); | ||
public async openProjects(projectFiles: vscode.Uri[]): Promise<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.
do we have a picker for projects like we do slns?
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 realize after I write this, a project picker for projects may not make sense, especially in huge workspaces. Though I am curious - what happens if we load a project that depends on another project via project reference (and the dependency is not loaded)?
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.
@dibarbet I'm not including a picker in this PR. Omnisharp did have one but in the case of a large workspace I'm not entirely sure how useful that experience would have been, and at least one customer so far has pointed out they were switching projects when they probably didn't mean to.
If the dependency isn't there, we'll just treat it as a metadata reference and load the DLL.
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.
Gotcha. Lets make sure that experience is tracked as well. I agree it could be complicated - for example it would be nice if you once you select the projects to load you can save it somewhere (e.g. similar to default solution). Or maybe instead we just implement load projects on demand... Not sure either.
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'd hope projects-on-demand meets many needs, or yes, some other way to specify the list of projects (probably via globbing.)
84bde48
to
bd275d5
Compare
Does this work for both blue and green? |
@CyrusNajmabadi This behavior only changes the behavior for the base C# extension. DevKit has separate support for dealing with a missing sln (right now it generates one for you.) The Dev Kit owners are definitely working on fixing that and we'll probably have some more work here to better unify it. |
Fixes #5923