Skip to content

Conversation

@Nigusu-Allehu
Copy link
Member

@Nigusu-Allehu Nigusu-Allehu commented Oct 14, 2025

Bug

Fixes:

Description

This PR introduces a generic PackageArgument type. Previously, we had Package type which only supported VersionRange versions. This new generic enables callers to implement it with both NuGetVersion and VersionRange.

PR Checklist

  • Meaningful title, helpful description and a linked NuGet/Home issue
  • Added tests
  • Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

@Nigusu-Allehu Nigusu-Allehu marked this pull request as ready for review October 14, 2025 23:49
@Nigusu-Allehu Nigusu-Allehu requested a review from a team as a code owner October 14, 2025 23:49
@Nigusu-Allehu Nigusu-Allehu marked this pull request as draft October 14, 2025 23:50

namespace NuGet.CommandLine.XPlat.Utility
{
internal static class PackageArgumentFactoryUtility
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The factory isn't needed for generics, instead you can create a static method on the class and have it create typed instances

https://dotnetfiddle.net/jFHQhx

public static PackageArgument<TVersion> Create(string id, TVersion version, IEqualityComparer<TVersion?> versionComparer)
{
    return new PackageArgument<TVersion>(versionComparer)
	{
	    Id = id,
        Version = version,
    };
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. that’s a really good point. Having a static method on the generic class would definitely simplify the design and make it more flexible.

The reason I introduced the factory, though, was to make it easier for callers so they don’t have to remember to pass in the comparer every time. It ensures consistency by automatically using the right comparer for NuGetVersion and VersionRange cases.

@Nigusu-Allehu
Copy link
Member Author

Thank you for the reviews. I will be closing this PR since we went ahead with the other implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants