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.

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

Move off of the legacy IVsPackageInstallerServices.IsPackageInstalled API. #53399

Merged
merged 5 commits into from
May 13, 2021

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented May 13, 2021

Nuget has no plans to provide a more moden equivalent. The recommendation is to just use GetInstalledPackagesAsync and check the value of that. So that's what this PR does :)

Should be viewed with whitespace diff off.

}
}

return false;
}

private bool IsPackageInstalled(ProjectId projectId, string packageName)
{
var cancellationToken = CancellationToken.None;
Copy link
Member Author

Choose a reason for hiding this comment

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

we coudl consdier actually making this codepath cancellable with a TWD. but that felt out of scope here (And isn't something we supported prior to this either).

@CyrusNajmabadi
Copy link
Member Author

I have manually validated all the scenairos (including undo/redo). Everythign seems to be working properly. There's an open case that we should have install/uninstall use a TWD, but we've lived without that for now, so i'm fine not changing that here.

cancellationToken.ThrowIfCancellationRequested();
await ProcessProjectChangeAsync(nugetService, solution, projectId, cancellationToken).ConfigureAwait(false);
}
return await doWorkAsync(nugetService).ConfigureAwait(false);
Copy link
Member

Choose a reason for hiding this comment

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

would it be reasonable here to make the function just take in a cancellation token? To avoid any chance that the cancellation token in dowork is different from the one passed into PerformNuGetProjectServiceWorkAsync

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, that woudl be reasonable :)

Copy link

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

Looks pretty good.
Only consideration is whether you check for top level only, or all.

@CyrusNajmabadi CyrusNajmabadi merged commit f63fd0f into dotnet:main May 13, 2021
@ghost ghost added this to the Next milestone May 13, 2021
@CyrusNajmabadi CyrusNajmabadi deleted the isPackageInstalled branch May 13, 2021 22:46
@RikkiGibson RikkiGibson modified the milestones: Next, 17.0.P2 Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants