Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add restore support to the language server #70588

Merged
merged 4 commits into from
Oct 30, 2023
Merged

Add restore support to the language server #70588

merged 4 commits into from
Oct 30, 2023

Conversation

dibarbet
Copy link
Member

@dibarbet dibarbet commented Oct 26, 2023 •
edited
Loading

First part of dotnet/vscode-csharp#5725

Adds support on the server side for

  1. A handler to retrieve the set of possible projects to restore. This is used to show a project picker for the project to restore on the client side.
  2. A handler to restore a specified project (or all projects). This is used once a project is picked from 1), or for the restore all command on the client side.

Followups

  1. Fixing file watching to include project.assets.json (ProjectAssetsFile)
  2. Integration tests on the client
  3. Automatic nuget restore detection

Client side PR - dotnet/vscode-csharp#6603

restore_projects

@dibarbet dibarbet requested a review from a team as a code owner October 26, 2023 23:03
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Issues and PRs which have not yet been triaged by a lead labels Oct 26, 2023
@dibarbet dibarbet mentioned this pull request Oct 26, 2023
Merged
CyrusNajmabadi
CyrusNajmabadi reviewed Oct 27, 2023
View reviewed changes
...Microsoft.CodeAnalysis.LanguageServer/LanguageServer/Handler/Restore/RestorePartialResult.cs

[DataContract]
internal record RestorePartialResult(
[property: DataMember(Name = "stage")] string Stage,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does 'stage' mean here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stage indicates what overall step is happening. This value is shown in the progress bar (whereas the messages are only shown in the output window)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see. can you doc. It wasn't clear. Looking at the PR it makes more sense now. It allows the messages to be bucketed together right? So you can tell which project these are associated with?

@dibarbet dibarbet merged commit 79d1884 into dotnet:main Oct 30, 2023
24 checks passed
@dibarbet dibarbet deleted the nuget_restore branch October 30, 2023 19:58
@ghost ghost added this to the Next milestone Oct 30, 2023
jasonmalinowski
jasonmalinowski reviewed Oct 30, 2023
View reviewed changes
...soft.CodeAnalysis.LanguageServer/LanguageServer/Handler/Restore/RestorableProjectsHandler.cs Show resolved Hide resolved
...soft.CodeAnalysis.LanguageServer/LanguageServer/Handler/Restore/RestorableProjectsHandler.cs Show resolved Hide resolved
...soft.CodeAnalysis.LanguageServer/LanguageServer/Handler/Restore/RestorableProjectsHandler.cs Show resolved Hide resolved
...erver/Microsoft.CodeAnalysis.LanguageServer/LanguageServer/Handler/Restore/RestoreHandler.cs Show resolved Hide resolved
...erver/Microsoft.CodeAnalysis.LanguageServer/LanguageServer/Handler/Restore/RestoreHandler.cs Show resolved Hide resolved
...erver/Microsoft.CodeAnalysis.LanguageServer/LanguageServer/Handler/Restore/RestoreHandler.cs Show resolved Hide resolved
...erver/Microsoft.CodeAnalysis.LanguageServer/LanguageServer/Handler/Restore/RestoreHandler.cs

if (process.ExitCode != 0)
{
throw new InvalidOperationException(LanguageServerResources.Failed_to_run_restore_see_output_for_details);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also get reported through the other progress reporter, since I'd assume failure to restore is mostly a user problem?

...erver/Microsoft.CodeAnalysis.LanguageServer/LanguageServer/Handler/Restore/RestoreHandler.cs
Comment on lines +88 to +90
// No file paths were specified - this means we should restore all projects in the solution.
// If there is a valid solution path, use that as the restore path.
if (solution.FilePath != null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't need a change now, but we should still think about what happens if the user has a solution file and also has load on demand enabled, since this wouldn't cover the load-on-demand projects.

@dibarbet dibarbet mentioned this pull request Oct 30, 2023
Merged
@jjonescz jjonescz modified the milestones: Next, 17.9 P1 Oct 31, 2023
@allisonchou allisonchou mentioned this pull request Nov 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

@jasonmalinowski jasonmalinowski jasonmalinowski left review comments

Assignees
No one assigned
Labels
Area-Infrastructure untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
17.9 P1
Development

Successfully merging this pull request may close these issues.
4 participants
@dibarbet @jasonmalinowski @CyrusNajmabadi @jjonescz