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

SlnFileFactory.CreateFromFilteredSolutionFile: Refactor #46404

Merged
merged 4 commits into from
Feb 3, 2025

Conversation

edvilme
Copy link
Member

@edvilme edvilme commented Jan 29, 2025

Refactored to simplify code and improve readability

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-CLI untriaged Request triage from a team member labels Jan 29, 2025
@edvilme edvilme changed the title SlnFileFactory.CreateFromFilteredSolutionFile: Use JsonNode SlnFileFactory.CreateFromFilteredSolutionFile: Refactor Feb 3, 2025
Comment on lines +95 to +97
// Store the original solution path in the description field of the filtered solution
filteredSolution.Description = originalSolutionPathAbsolute;

Copy link
Member Author

Choose a reason for hiding this comment

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

In some areas it might be useful to keep a reference to the original solution file path. See #46392
The Description field is used to store manually entered user descriptions for solution files. This feature does not exist in .slnf schema, so it seems reasonable to describe a solution filter by its original solution file path.
Doing this also avoids having to parse the file twice to find this data.

Copy link
Member

Choose a reason for hiding this comment

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

If Description doesn't exist in the .slnf schema, does that mean this value only exists in memory? If it isn't in the schema, it would never get written to disk. So, what is the purpose of setting it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, at the moment this does not write to slnf files. However, it is useful having a field in the SolutionModel that stores metadata such as Description. For example, on

string solution = SlnFileFactory.GetSolutionPathFromFilteredSolutionFile(solutionFilterFilePath);

the file is parsed twice (once to get the projects, and a second time to get the original solution path). By storing this information in memory, this could be avoided.

We also wouldn't have to worry about overriding user-entered information as it is not part of the slnf file, but does describe it

Copy link
Member

@MiYanni MiYanni left a comment

Choose a reason for hiding this comment

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

Changes generally look good. One question on the Description. Also, are there tests that exercise this method? Just want to make sure the changes don't affect the logic.

Comment on lines +95 to +97
// Store the original solution path in the description field of the filtered solution
filteredSolution.Description = originalSolutionPathAbsolute;

Copy link
Member

Choose a reason for hiding this comment

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

If Description doesn't exist in the .slnf schema, does that mean this value only exists in memory? If it isn't in the schema, it would never get written to disk. So, what is the purpose of setting it?

@edvilme
Copy link
Member Author

edvilme commented Feb 3, 2025

Changes generally look good. One question on the Description. Also, are there tests that exercise this method? Just want to make sure the changes don't affect the logic.

Yes, a test for listing projects in a solution filter exists in

public void WhenSolutionFilterIsPassedItListsProjectsMatching(string solutionCommand)

@edvilme edvilme merged commit e6366a5 into dotnet:release/9.0.3xx Feb 3, 2025
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CLI untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants