Skip to content

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

Merged
CyrusNajmabadi merged 5 commits intodotnet:mainfrom
CyrusNajmabadi:isPackageInstalled
May 13, 2021
Merged

Move off of the legacy IVsPackageInstallerServices.IsPackageInstalled API.#53399
CyrusNajmabadi merged 5 commits intodotnet:mainfrom
CyrusNajmabadi:isPackageInstalled

Conversation

@CyrusNajmabadi
Copy link
Contributor

@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.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner May 13, 2021 19:55
@ghost ghost added the Area-IDE label May 13, 2021

private bool IsPackageInstalled(ProjectId projectId, string packageName)
{
var cancellationToken = CancellationToken.None;
Copy link
Contributor 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
Contributor 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
Contributor 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