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

Remove SlnFile references #45442

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

Conversation

edvilme
Copy link
Member

@edvilme edvilme commented Dec 12, 2024

SlnFile is being replaced by vs-solutionpersistence as part of the efforts for .slnx support
This should be replaced in all remaining places it might be used

Contributes #40913

@@ -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) => [
Copy link
Contributor

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

@edvilme edvilme requested a review from a team January 8, 2025 22:13
@edvilme edvilme marked this pull request as ready for review January 8, 2025 22:13
@edvilme edvilme requested review from a team as code owners January 8, 2025 22:13
@edvilme edvilme requested review from a team and vijayrkn as code owners January 8, 2025 22:13
@edvilme edvilme changed the title (Draft) Remove SlnFile Remove SlnFile references Jan 8, 2025
Copy link
Member

@Forgind Forgind left a 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
Copy link
Member

Choose a reason for hiding this comment

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

TODO

src/Cli/dotnet/ReleasePropertyProjectLocator.cs Outdated Show resolved Hide resolved
{
#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);
Copy link
Member

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);
Copy link
Member

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?

Copy link
Member Author

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");
Copy link
Member

Choose a reason for hiding this comment

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

or slnf?

Copy link
Member Author

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

Copy link
Member

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?

Copy link
Member Author

@edvilme edvilme Jan 9, 2025

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

microsoft/vs-solutionpersistence#86

src/Cli/dotnet/SlnFileFactory.cs Show resolved Hide resolved
@@ -78,6 +78,7 @@ public override int Execute()
{
throw new GracefulException(CommonLocalizableStrings.InvalidSolutionFormatString, solutionFileFullPath, ex.Message);
}
// TODO: Check
Copy link
Member

Choose a reason for hiding this comment

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

TODO

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

Successfully merging this pull request may close these issues.

3 participants