Fix TFM ordering in Dependencies tab using NuGetFrameworkSorter#10655
Fix TFM ordering in Dependencies tab using NuGetFrameworkSorter#10655
Conversation
… sorting Co-authored-by: joelverhagen <94054+joelverhagen@users.noreply.github.com>
…mework comparer Co-authored-by: joelverhagen <94054+joelverhagen@users.noreply.github.com>
Co-authored-by: joelverhagen <94054+joelverhagen@users.noreply.github.com>
Fixed the test expectations. The |
Co-authored-by: joelverhagen <94054+joelverhagen@users.noreply.github.com>
tests/NuGetGallery.Facts/ViewModels/DependencySetsViewModelFacts.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: joelverhagen <94054+joelverhagen@users.noreply.github.com>
|
I validated this change locally. Good robot. |
| { | ||
| if (!DependencySets.ContainsKey(group.friendlyName)) | ||
| { | ||
| DependencySets.Add(group.friendlyName, group.dependencies); |
There was a problem hiding this comment.
I'm very confused how this works. Why does DependencySets.Keys return the keys ordered how you expect them to?
There was a problem hiding this comment.
I think you need to address (at)copilot to get a response.
I missed this, I had assumed this was a collection that supported the order as we'd expect. I think this is working because of a Dictionary implementation detail -- not good.
There was a problem hiding this comment.
@copilot - can you check this comment thread please?
There was a problem hiding this comment.
You're correct that relying on Dictionary insertion order was problematic, especially for .NET Framework 4.7.2 where it's not guaranteed. Changed DependencySets from IDictionary<string, IEnumerable<DependencyViewModel>> to IReadOnlyList<KeyValuePair<string, IEnumerable<DependencyViewModel>>> to explicitly preserve the sorted order. The sorted frameworks are now stored in a List which guarantees insertion order, making the ordering contract explicit. (7a1b3c0)
…ing on Dictionary implementation Co-authored-by: joelverhagen <94054+joelverhagen@users.noreply.github.com>
Co-authored-by: joelverhagen <94054+joelverhagen@users.noreply.github.com>
|
@copilot check these build errors |
Co-authored-by: joelverhagen <94054+joelverhagen@users.noreply.github.com>
| NuGetFramework framework; | ||
|
|
||
| if (!DependencySets.ContainsKey(targetFramework)) | ||
| if (tfmString == null) |
There was a problem hiding this comment.
string.IsNullOrEmpty(tfmString )
Fix TFM Ordering in Dependencies Tab ✅
Changes Summary
Problem: The Dependencies tab was sorting TFMs (Target Framework Monikers) lexicographically by their friendly name strings, causing
net10.0to appear beforenet8.0andnet9.0.Solution: Modified
DependencySetsViewModelto:NuGetFrameworkobjectsNuGetFrameworkSorter.Instancewhich properly handles framework identifier and versionNullableFrameworkComparerinstance via static readonly property for better performanceIReadOnlyList<KeyValuePair<...>>to explicitly guarantee ordering (not relying on Dictionary implementation details)Test Coverage:
GivenAListOfDependenciesNet10WillBeOrderedAfterNet9test to verify correct ordering of net8.0, net9.0, and net10.0GivenAListOfDependenciesPackageIdsWillBeOrderedCaseInsensitivelytest to verify case-insensitive package ID sortingToFriendlyName()behavior for modern .NET TFMs (net8.0, net9.0, net10.0 remain as-is)Security Summary
No security vulnerabilities were introduced by these changes.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.