Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support package@version syntax in dotnet new install, partial fix for #45422 #45545

Open
wants to merge 3 commits into
base: release/9.0.2xx
Choose a base branch
from
Open

Support package@version syntax in dotnet new install, partial fix for #45422 #45545

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d1aa890
Support split-by-at
Forgind Dec 18, 2024
50d556e
Support stupid test
Forgind Dec 19, 2024
00bba93
Change split mechanism and add test
Forgind Dec 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions src/Cli/Microsoft.TemplateEngine.Cli/TemplatePackageCoordinator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,9 +207,14 @@ internal async Task<NewCommandStatus> EnterInstallFlowAsync(InstallCommandArgs a

foreach (string installArg in args.TemplatePackages)
{
string[] splitByColons = installArg.Split(new[] { "::" }, StringSplitOptions.RemoveEmptyEntries);
string identifier = splitByColons[0];
string? version = splitByColons.Length > 1 ? splitByColons[1] : null;
string[] splitByColons = installArg.Split(["::"], StringSplitOptions.RemoveEmptyEntries);
string[] splitByAt = installArg.Split('@', StringSplitOptions.RemoveEmptyEntries);
string[] split = splitByColons.Length > splitByAt.Length ? splitByColons :
splitByAt.Length > splitByColons.Length ? splitByAt :
Copy link
Member Author

Choose a reason for hiding this comment

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

Why is this monstrosity necessary?

I wanted to avoid breaking changes. If one array is longer, that means it split more, and that means we should use it. If the arrays are the same length but the thing at the first index is shorter in one than the other, that means it parsed out (and dropped) (a) separator character(s), and we should use the shorter one.

We should really change CanInstallRemoteNuGetPackage_LatestVariations when we can finally get rid of support for ::

Copy link
Member

Choose a reason for hiding this comment

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

Are you ever supposed to have more than one instance of either :: or @ in the argument?

It seems like you could check if the string contains ::. If it does, split on that, otherwise try splitting it on @.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm...there could be multiple...which also makes me wonder whether anyone would try dotnet new install package1::version1 package2@version2. The semi-reasonable reason someone might do that is if they have some script that takes optional 'additional' package@version things and runs dotnet new install with some 'base' package::version things.

What would you think of something like:
string[] split = installArgs.Split(["::"], StringSplitOptions.RemoveEntryEntries).Select(arg => arg.Split('@', StringSplitOptions.RemoveEmptyEntries)).Flatten().ToArray();

? That should support both cases.

splitByColons[0].Length < splitByAt[0].Length ? splitByColons :
splitByAt;
string identifier = split[0];
string? version = split.Length > 1 ? split[1] : null;
foreach (string expandedIdentifier in InstallRequestPathResolution.ExpandMaskedPath(identifier, _engineEnvironmentSettings))
{
installRequests.Add(new InstallRequest(expandedIdentifier, version, details: details, force: args.Force));
Expand Down
Loading