Skip splitting for paths Fixes #48794 #48874
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issue #48794 by preventing unintended splitting of package paths.
- Introduces a check to determine if a package argument is a valid file or directory.
- Uses this check to conditionally skip path splitting, treating the entire argument as a package path if it exists.
| @@ -208,7 +208,8 @@ internal async Task<NewCommandStatus> EnterInstallFlowAsync(InstallCommandArgs a | |||
|
|
|||
| foreach (string installArg in args.TemplatePackages) | |||
| { | |||
There was a problem hiding this comment.
Consider adding a comment to explain that the check is used to skip splitting for arguments that represent existing file or directory paths, which will help future maintainers understand the purpose of this logic.
| { | |
| { | |
| // Check if the argument represents an existing file or directory path. | |
| // If it is a path, skip splitting the argument, as it is already in the desired format. |
Copilot uses AI. Check for mistakes.
| @@ -208,7 +208,8 @@ internal async Task<NewCommandStatus> EnterInstallFlowAsync(InstallCommandArgs a | |||
|
|
|||
| foreach (string installArg in args.TemplatePackages) | |||
| { | |||
| string[] split = installArg.Split(["::"], StringSplitOptions.RemoveEmptyEntries).SelectMany(arg => arg.Split('@', StringSplitOptions.RemoveEmptyEntries)).ToArray(); | |||
| bool isPath = File.Exists(installArg) || Directory.Exists(installArg); | |||
There was a problem hiding this comment.
Isn't there something to check if it is a path? Like, something related to that RelativePath utility thing. Because you shouldn't need to check if something is on-disk to know if the argument is a path or not.
There was a problem hiding this comment.
We could call http://msdn.microsoft.com/en-us/library/system.io.path.getfullpath.aspx Path.GetFullPath and catch the exception to see if it's not a path?
There was a problem hiding this comment.
I don't know that I agree with this. If you want to dotnet new install something in your current directory, it would be very hard to tell (without checking if it exists) that it's a path.
There was a problem hiding this comment.
And if you were to just check for an exception from Path.GetFullPath, I think ~any package would show up as a path
Test added. The temp path should be OS-specific to some extent, so I think that would cover all those cases and would mean that you don't need OS-specific Facts. |
Fixes #48794