Fix https://github.com/NuGet/Home/issues/1816 #501
Conversation
|
@jainaashish Can you add test(s) to illustrate the issue and the fix? |
|
This seems correct. I would double check that no tests are failing, and verify that if there are no packages in the allowed range that it gives a good error message for that instead of saying that it cannot find the package at all. In the past I've found that small changes to order here can change the many errors displayed. |
jainaashish
force-pushed
the
jainaashish/updateFails
branch
from
a81db7e
to
a5ffd72
Compare
|
So i've updated the PM UI to consider allowedVersion range defined in package.config to show updates available. Also added/updated tests for the issue fixed. new UX will look like this: |
|
@emgarten @alpaix @rrelyea @yishaigalatzer @harikm86 |
|
We should not allow update of a package to a version that is outside of the bounds of the version constraints. I can see value in selecting a version that is below the version constraints not satisfied header so the user can check out release notes or whatever, but we shouldn't allow the update. We should disable the button and possibly have a message explaining why that is the case. |
|
There are cases wants to override the constraints, so we need to consider that blocking updates will cause undesired side effects (which is why we already have the ignore dependencies check box). So please take time and design the experience end to end. Justin can help with advice |
|
As discussed with Hari and Justin, we finalized that we'll disable blocked versions to update or install until user changes dependency behavior. If dependency behavior is changed to ignore, then we'll enable these blocked versions to update or install. |
| @@ -569,13 +569,13 @@ protected IEnumerable<IPackageSearchMetadata> GetPackagesFromRemoteSource(string | |||
|
|
|||
| protected async Task<IEnumerable<IPackageSearchMetadata>> GetPackagesFromRemoteSourceAsync(string packageId, bool includePrerelease) | |||
| { | |||
| var metadataProvider = new MultiSourcePackageMetadataProvider(PrimarySourceRepositories, optionalLocalRepository: null, optionalGlobalLocalRepository: null, logger: Common.NullLogger.Instance); | |||
| var metadataProvider = new MultiSourcePackageMetadataProvider(PrimarySourceRepositories, optionalLocalRepository: null, optionalGlobalLocalRepository: null, projects: new NuGetProject[] { Project }, isSolution:false, logger: Common.NullLogger.Instance); | |||
There was a problem hiding this comment.
nit: format the document, the spacing looks off here
|
AllowedVersions has never been a first class feature. This is a large change which still makes it only a partial feature. It still doesn't have a way to modify or add the ranges from the UI or even the console. I'm not sure that the increased UI complexity here will benefit the number advanced users manually editing their packages.config to add this attribute. Looking at the original bug the user is using nuget.exe, there haven't been any user requests for this. ⌚ |
jainaashish
force-pushed
the
jainaashish/updateFails
branch
from
a5ffd72
to
b244dd7
Compare
|
As discussed offline, we'll continue with the fix but will make sure we've a manual UI test added to test the feature. Besides I've already added test to cover original bug for pruning versions based on allowedVersions before choosing the highest available. |
|
|
||
| // Remove zero width ranges. Ex: (1.0.0, 1.0.0) | ||
| // This includes VersionRange.None and any other ranges that satisfy zero versions | ||
| ranges = ranges.Where(range => HasValidRange(range)); |
There was a problem hiding this comment.
Invalid ranges should cause this method to return None instead of being ignored. Add a test for this also
|
⌚ simplify CommonSubSet and add more tests |
Removing versions whish don't satisfy allowedVersions before picking the highest version
jainaashish
force-pushed
the
jainaashish/updateFails
branch
from
b244dd7
to
6c4bf7b
Compare
|
LGTM 🚀 |
|
Merged 2cc8fc0 |
Removing versions which don't satisfy allowedVersions before picking the highest version
Fixes: NuGet/Home#1816