Fix version handling in `dotnet add file.cs package` #50924

jjonescz · 2025-09-19T12:11:28Z

Copilot

Pull Request Overview

This PR fixes version handling in the dotnet add file.cs package command by addressing an issue where the --version option was not properly considered when determining if a version was specified.

Replaces the hasVersion boolean logic with a unified specifiedVersion string that captures both @version syntax and --version option values
Refactors file-based package add tests to use parameterized testing with combinatorial data to cover more command variations
Consolidates duplicate test methods into a single helper method that generates appropriate command arguments

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/Cli/dotnet/Commands/Package/Add/PackageAddCommand.cs	Updates version detection logic to consider both `@version` syntax and `--version` option
test/dotnet.Tests/CommandTests/Package/Add/GivenDotnetPackageAdd.cs	Refactors tests to use combinatorial data and shared helper method for better coverage

src/Cli/dotnet/Commands/Package/Add/PackageAddCommand.cs

test/dotnet.Tests/CommandTests/Package/Add/GivenDotnetPackageAdd.cs

jjonescz · 2025-09-19T17:26:08Z

@RikkiGibson @333fred @MiYanni for reviews, thanks

nkolev92 · 2025-09-19T19:45:51Z

src/Cli/dotnet/Commands/Package/Add/PackageAddCommand.cs

-            : (prerelease
-                ? "*-*"
-                : "*");
+        string version = specifiedVersion ?? (prerelease ? "*-*" : "*");


fyi this will add unlisted packages as well NuGet/Home#7466

Just in case you get an issue around it.

Referencing NuGet/Home#14390 to make it easier for when NuGet starts operating on msbuild projects.

So wildcards/floats will resolve unlisted versions? That's undesirable. Is this also true for regular <PackageReference /> items, or just in the CLIs' add commands?

That's how floating versions work in general.

The trick is that file based add package is using a floating version to determine the latest version, while regular add package does a separate call.

while regular add package does a separate call.

So should we be doing the same for file-based apps then?

Ideally yes, but the issue NuGet/Home#14390 which @nkolev92 linked tracks moving this logic to NuGet so that will resolve the problem.

jjonescz · 2025-09-22T08:52:37Z

@RikkiGibson @333fred @MiYanni for reviews, thanks

baronfel · 2025-09-22T13:36:23Z

src/Cli/dotnet/Commands/Package/Add/PackageAddCommand.cs

-        bool hasVersion = _packageId.HasVersion;
+        string? specifiedVersion = _packageId.HasVersion
+            ? _packageId.VersionRange?.OriginalString ?? string.Empty
+            : _parseResult.GetValue(PackageAddCommandParser.VersionOption);


nit: This makes me think that we should change the VersionOption to be a proper type instead of a string. We should be tightening these things up as we go.

baronfel · 2025-09-22T13:41:02Z

test/dotnet.Tests/CommandTests/Package/Add/GivenDotnetPackageAdd.cs

        {
+            if (!legacyForm && !fileOption)
+            {
+                Log.WriteLine("Skipping invalid combination of parameters");


nit: one thing I really love about TUnit is you can declare a specific combination of test theory parameters as an exclusion. Wish we had that here. Would be lovely to be able to use xunitv3 or something more modern.

baronfel · 2025-09-22T13:41:21Z

test/dotnet.Tests/CommandTests/Package/Add/GivenDotnetPackageAdd.cs


-        [Fact]
-        public void FileBasedApp_CentralPackageManagement_NoVersionSpecified_KeepExisting()
+        [Theory, CombinatorialData]


love to see the more theory-driven samples here!

There isn't a need to make a change in this PR, but, I would caution about complex use of CombinatorialData, and potential effect on execution time of the whole suite over time. 32 cases for a theory is far from the worst in that regard--that's the most I think I saw in this PR--but, compiler tests have inadvertently created 1000 or so combinatorial cases for a single theory in the past, IIRC.

But, if we don't have a reason to think that every single combination of the arguments is exercising an interesting aspect of the system, it can be worthwhile to consider methods like pairwise testing. This can be a good supplement to determining the cases which are thought to be interesting solely based on the meaning of the respective inputs.

It looks like Andrew actually implemented pairwise testing in the same library that implements CombinatorialDataAttribute: https://aarnott.github.io/Xunit.Combinatorial/docs/combinatorial-vs-pairwise.html#pairwise-testing.

RikkiGibson · 2025-09-22T16:35:22Z

src/Cli/dotnet/Commands/Package/Add/PackageAddCommand.cs

-        bool hasVersion = _packageId.HasVersion;
+        string? specifiedVersion = _packageId.HasVersion
+            ? _packageId.VersionRange?.OriginalString ?? string.Empty
+            : _parseResult.GetValue(PackageAddCommandParser.VersionOption);


Is anything blocking both CmdPackageArgument and VersionOption specifying a version at once?

Yes, there is a validator on the VersionOption for that.

RikkiGibson · 2025-09-22T16:50:02Z

test/dotnet.Tests/CommandTests/Package/Add/GivenDotnetPackageAdd.cs

-        public void FileBasedApp()
+        private string[]? GetFileBasedAppArgs(bool legacyForm, bool? versionOption, bool fileOption, bool noRestore, string packageName = "Humanizer")
        {
+            if (!legacyForm && !fileOption)


Does this mean that when the "legacy form" add package is used, it can automatically find and update Program.cs?

No, but it can specify the file as an argument (dotnet add file.cs package) whereas the current form can specify it only as an option (dotnet package add --file file.cs)

RikkiGibson · 2025-09-22T17:13:01Z

test/dotnet.Tests/CommandTests/Package/Add/GivenDotnetPackageAdd.cs


-        [Fact]
-        public void FileBasedApp_CentralPackageManagement_NoVersionSpecified_KeepExisting()
+        [Theory, CombinatorialData]


There isn't a need to make a change in this PR, but, I would caution about complex use of CombinatorialData, and potential effect on execution time of the whole suite over time. 32 cases for a theory is far from the worst in that regard--that's the most I think I saw in this PR--but, compiler tests have inadvertently created 1000 or so combinatorial cases for a single theory in the past, IIRC.

But, if we don't have a reason to think that every single combination of the arguments is exercising an interesting aspect of the system, it can be worthwhile to consider methods like pairwise testing. This can be a good supplement to determining the cases which are thought to be interesting solely based on the meaning of the respective inputs.

Fix version handling in dotnet add file.cs package

3b760e7

jjonescz added the Area-run-file Items related to the "dotnet run <file>" effort label Sep 19, 2025

jjonescz marked this pull request as ready for review September 19, 2025 13:29

jjonescz requested a review from a team as a code owner September 19, 2025 13:29

jjonescz requested review from a team and Copilot September 19, 2025 13:29

Copilot AI reviewed Sep 19, 2025

View reviewed changes

src/Cli/dotnet/Commands/Package/Add/PackageAddCommand.cs Show resolved Hide resolved

test/dotnet.Tests/CommandTests/Package/Add/GivenDotnetPackageAdd.cs Show resolved Hide resolved

build-analysis bot mentioned this pull request Sep 19, 2025

GenerateBundle Task fails writing to singlefilehost.exe #50784

Open

nkolev92 reviewed Sep 19, 2025

View reviewed changes

build-analysis bot mentioned this pull request Sep 22, 2025

dotnet-watch tests failing #40006

Open

baronfel approved these changes Sep 22, 2025

View reviewed changes

baronfel enabled auto-merge (squash) September 22, 2025 13:41

jjonescz disabled auto-merge September 22, 2025 13:45

RikkiGibson approved these changes Sep 22, 2025

View reviewed changes

jjonescz requested a review from 333fred September 22, 2025 17:19

333fred approved these changes Sep 22, 2025

View reviewed changes

333fred merged commit d69a91d into dotnet:release/10.0.1xx Sep 22, 2025
29 checks passed

jjonescz deleted the sprint-package-add branch September 23, 2025 05:29

Fix version handling in dotnet add file.cs package #50924

Fix version handling in dotnet add file.cs package #50924

Conversation

jjonescz commented Sep 19, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

jjonescz commented Sep 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjonescz commented Sep 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Fix version handling in `dotnet add file.cs package` #50924

Fix version handling in `dotnet add file.cs package` #50924