-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Remove SlnFile
references
#45442
base: release/9.0.2xx
Are you sure you want to change the base?
Remove SlnFile
references
#45442
Conversation
f367673
to
e1d98ce
Compare
ab898f4
to
1627d33
Compare
@@ -76,8 +76,11 @@ private static (bool isSolution, string workspacePath) FindFile(string workspace | |||
return (isSolution, workspacePath); | |||
} | |||
|
|||
private static IEnumerable<string> FindSolutionFiles(string basePath) => Directory.EnumerateFileSystemEntries(basePath, "*.sln", SearchOption.TopDirectoryOnly) | |||
.Concat(Directory.EnumerateFileSystemEntries(basePath, "*.slnf", SearchOption.TopDirectoryOnly)); | |||
private static IEnumerable<string> FindSolutionFiles(string basePath) => [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can return string[] and use the array all the way to avoid .ToList in the end. it makes an unnecessary copy in FindMatchingFile to find count
Co-authored-by: kasperk81 <[email protected]>
258a269
to
24b44dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lot of deleted code! I guess it was all SlnFile-specific?
I think this at least mostly accomplishes your goal. I feel like we should better support slnf, as that's basically a sln*.
@@ -22,7 +21,8 @@ public static string GetDefaultProjectTypeGuid(this ProjectInstance projectInsta | |||
string projectTypeGuid = projectInstance.GetPropertyValue("DefaultProjectTypeGuid"); | |||
if (string.IsNullOrEmpty(projectTypeGuid) && projectInstance.FullPath.EndsWith(".shproj", StringComparison.OrdinalIgnoreCase)) | |||
{ | |||
projectTypeGuid = ProjectTypeGuids.SharedProjectGuid; | |||
// TODO: Centralize project type guids |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO
{ | ||
#pragma warning disable CS8604 // Possible null reference argument. | ||
string projectFullPath = Path.Combine(Path.GetDirectoryName(sln.FullPath), project.FilePath); | ||
string projectFullPath = Path.Combine(Path.GetDirectoryName(slnPath), project.FilePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this method (in code that was from before this PR) do a Parallel.ForEach on a bunch of projects when you just want one? Seems wasteful
{ | ||
return project.TypeGuid == solutionFolderGuid || project.TypeGuid == sharedProjectGuid || !IsValidProjectFilePath(projectFullPath); | ||
return project.TypeId.ToString() == solutionFolderGuid || project.TypeId.ToString() == sharedProjectGuid || !IsValidProjectFilePath(projectFullPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is TypeId always the same guid as it would've been had you used TypeGuid for a SlnProject?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean if they're equivalent? In that case, yes
@@ -262,7 +271,7 @@ private bool IsValidProjectFilePath(string path) | |||
/// <returns>Returns true if the path exists and is a sln file type.</returns> | |||
private bool IsValidSlnFilePath(string path) | |||
{ | |||
return File.Exists(path) && Path.GetExtension(path).EndsWith("sln"); | |||
return File.Exists(path) && (Path.GetExtension(path) == ".sln" || Path.GetExtension(path) == ".slnx"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or slnf?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this does not currently support solution filter files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but I guess my question is: why not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could wait and see if/when vs-solutionpersistence implements support for slnf files before trying to parse them on our own
src/Cli/dotnet/commands/dotnet-list/dotnet-list-package/ListPackageReferencesCommand.cs
Outdated
Show resolved
Hide resolved
@@ -78,6 +78,7 @@ public override int Execute() | |||
{ | |||
throw new GracefulException(CommonLocalizableStrings.InvalidSolutionFormatString, solutionFileFullPath, ex.Message); | |||
} | |||
// TODO: Check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO
SlnFile
is being replaced by vs-solutionpersistence as part of the efforts for .slnx supportThis should be replaced in all remaining places it might be used
Contributes #40913