-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Move off of the legacy IVsPackageInstallerServices.IsPackageInstalled API. #53399
Conversation
} | ||
} | ||
|
||
return false; | ||
} | ||
|
||
private bool IsPackageInstalled(ProjectId projectId, string packageName) | ||
{ | ||
var cancellationToken = CancellationToken.None; |
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.
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).
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); |
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.
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
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, that woudl be reasonable :)
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.
Looks pretty good.
Only consideration is whether you check for top level only, or all.
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.