Setting NuGetPackageId for UnpkgProvider and filtering null NuGetPack… #154
Conversation
…ageId's from install steps.
raquelsa
requested review from
prafullbhosale,
alexgav,
jimmylewis,
jodavis and
bhsubra
| @@ -22,7 +22,7 @@ public UnpkgProvider(IHostInteraction hostInteraction) | |||
|
|
|||
| public string Id => IdText; | |||
|
|
|||
| public string NuGetPackageId => null; | |||
| public string NuGetPackageId { get; } = "Microsoft.Web.LibraryManager.Build"; | |||
There was a problem hiding this comment.
You could've just left the =>, but it doesn't really matter.
There was a problem hiding this comment.
Why do providers know anything about the NuGet package for restore? This seems like a broken design. Should this be fixed in the command instead?
There was a problem hiding this comment.
I think this is good Preview3 as the simplest fix, but in general I completely agree.
There was a problem hiding this comment.
Can't we just change the packageIds line in RestoreOnBuildCommand.cs to be "Microsoft.Web.LibraryManager.Build"?
There was a problem hiding this comment.
oh, but currently this allows a provider to specify another package... hm, maybe not then.
There was a problem hiding this comment.
So, this should be something along the lines of:
- By default, RestoreOnBuildCommand adds the default package.
- Providers can specify any other packages they require. Null is allowed, and does not add to the set.
Am I missing something? This still feels like a 5 minute fix and we don't have to track it for another change later.
There was a problem hiding this comment.
I would just leave as is for now. Not sure what was the intent of the original design. We can propose design changes later since this fix if for preview3
There was a problem hiding this comment.
Logged issue #155 for the design review
| @@ -22,7 +22,7 @@ public UnpkgProvider(IHostInteraction hostInteraction) | |||
|
|
|||
| public string Id => IdText; | |||
|
|
|||
| public string NuGetPackageId => null; | |||
| public string NuGetPackageId { get; } = "Microsoft.Web.LibraryManager.Build"; | |||
There was a problem hiding this comment.
Why do providers know anything about the NuGet package for restore? This seems like a broken design. Should this be fixed in the command instead?
…ageId's from install steps. (#154)
…ageId's from install steps.