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.

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

Conversation

Forgind
Copy link
Member

@Forgind Forgind commented Dec 18, 2024

Progress towards #45422

This adds support for package@version syntax without removing support for package::version syntax. I'll add a follow-up PR in main to add a warning, and then we can remove support for package::version syntax in .NET 11.

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.

Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

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

I think we should have tests for this, especially since we want to make sure we're not breaking anything.

Hopefully that's simple, since we just need to test how arguments are parsed.

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

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 @.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-dotnet new the item is related to dotnet new command untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants