Refactoring the method that fetches the project context in the Solution PM UI #6065
Conversation
…on PM UI to return only a single package to simplify the caller logic
| } | ||
| else | ||
| { | ||
| project.InstalledVersion = null; | ||
| project.PackageLevel = null; | ||
| project.PackageLevel = packageContext is ITransitivePackageReferenceContextInfo ? PackageLevel.Transitive : PackageLevel.TopLevel; |
There was a problem hiding this comment.
Why set PackageLevel outside of the helper method UpdateProjectInstallationInfo?
| } | ||
| else | ||
| { | ||
| project.InstalledVersion = null; | ||
| project.PackageLevel = null; | ||
| project.PackageLevel = packageContext is ITransitivePackageReferenceContextInfo ? PackageLevel.Transitive : PackageLevel.TopLevel; |
There was a problem hiding this comment.
May be out of scope for this PR. Looking at the is operator, I think ITransitivePackageReferenceContextInfo interface contract can be improved. Here are the reasons.
- AFAIK, there is a concept called "Transitive package" but no such thing called "Transitive package reference" in NuGet.
ITransitivePackageReferenceContextInfoinhertingIPackageReferenceContextInfointerface seems incorrect to me because it is primarily meant for obtaining "Transitive origins" for a package.
public interface IPackageReferenceContextInfo
{
PackageIdentity Identity { get; }
NuGetFramework? Framework { get; }
VersionRange? AllowedVersions { get; }
bool IsAutoReferenced { get; }
bool IsUserInstalled { get; }
bool IsDevelopmentDependency { get; }
VersionRange? VersionOverride { get; }
}
public interface ITransitivePackageContextInfo
{
PackageIdentity Identity { get; }
IEnumerable<IPackageReferenceContextInfo> TransitiveOrigins { get; }
}I am open for feedback and happy to create a tech debt tracking issue.
There was a problem hiding this comment.
Definitely out of scope for this refactor. Please open an issue that explains the alternative you propose. I'll add details on the issue about how this evolved such that Transitive Packages became a specialized PackageReferenceContextInfo. Thanks!
There was a problem hiding this comment.
Created tracking tech debt issue https://github.com/NuGet/Client.Engineering/issues/3032
Refactoring the method that fetches the project context in the Solution PM UI to return only a single package and simplify the caller logic
Bug
Fixes: NuGet/Home#13216
Description
Addressing feedback from the original pull request for the Transitive Packages in Solution PM UI feature: #6044 (comment). This is a better design to only return one package context object and simplifies the caller logic in determining if the package is top-level or transitive. Removed an outdated comment on the method, and renamed the method to more accurately reflect its purpose.
PR Checklist
Added tests- Refactoring existing method. No new functionality to test.