Skip to content

Conversation

@Youssef1313
Copy link
Member

@rainersigwald Does this sound reasonable? I think this public API is going to be useful in dotnet test implementation in SDK.

Copilot AI review requested due to automatic review settings October 23, 2025 11:03
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the solution project buildability logic by moving it from SolutionProjectGenerator into SolutionFile as a public API. The change introduces IsProjectBuildable() methods that determine whether a project in a solution should be built for a given configuration, making this logic reusable for other components like dotnet test.

Key changes:

  • Moved WouldProjectBuild() logic from SolutionProjectGenerator to SolutionFile.IsProjectBuildable()
  • Added public overload using default configuration for easier API consumption
  • Simplified method signatures by removing redundant ProjectConfigurationInSolution parameters

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/Build/Construction/Solution/SolutionProjectGenerator.cs Refactored to call SolutionFile.IsProjectBuildable() and removed parameter passing of ProjectConfigurationInSolution
src/Build/Construction/Solution/SolutionFile.cs Added new public IsProjectBuildable() API with implementation moved from SolutionProjectGenerator

{
return _solutionFilter?.Contains(FileUtilities.FixFilePath(projectFile)) != false;
}

Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

The public API method IsProjectBuildable lacks XML documentation comments. Since this is being introduced as a public API for use in dotnet test and other components, it should include documentation describing its purpose, parameters, return value, and example usage scenarios.

Suggested change
/// <summary>
/// Determines whether the specified project is buildable in the context of the solution's default configuration and platform.
/// </summary>
/// <param name="projectInSolution">The project within the solution to check for buildability.</param>
/// <returns>
/// <c>true</c> if the project is included in the build for the default solution configuration and platform; otherwise, <c>false</c>.
/// </returns>
/// <remarks>
/// This method is intended for use by tools such as <c>dotnet test</c> and other components that need to determine if a project should be built.
/// </remarks>
/// <example>
/// <code>
/// var solutionFile = SolutionFile.Parse("MySolution.sln");
/// foreach (var project in solutionFile.ProjectsInOrder)
/// {
/// if (solutionFile.IsProjectBuildable(project))
/// {
/// // Build the project
/// }
/// }
/// </code>
/// </example>

Copilot uses AI. Check for mistakes.

public bool IsProjectBuildable(ProjectInSolution projectInSolution)
=> IsProjectBuildable(projectInSolution, $"{GetDefaultConfigurationName()}|{GetDefaultPlatformName()}");

Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

The internal overload of IsProjectBuildable lacks XML documentation comments. Even for internal methods, documentation helps maintainability by explaining the selectedSolutionConfiguration parameter format and how it differs from the public overload.

Suggested change
/// <summary>
/// Determines if the specified project is buildable for the given solution configuration.
/// </summary>
/// <param name="projectInSolution">The project to check for buildability.</param>
/// <param name="selectedSolutionConfiguration">
/// The full solution configuration name in the format "Configuration|Platform" (e.g., "Debug|Any CPU").
/// This allows specifying a configuration/platform pair different from the default used by the public overload.
/// </param>
/// <returns>
/// True if the project is buildable for the specified solution configuration; otherwise, false.
/// </returns>

Copilot uses AI. Check for mistakes.
@Youssef1313 Youssef1313 force-pushed the dev/ygerges/solution-gen branch from 3826837 to 8c6422c Compare October 23, 2025 11:17
Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

I am trying really hard to hold the line on "do not add new functionality to our broken solution parser". VS owns solutions and should be providing this functionality in SolutionPersistence.

@Youssef1313
Copy link
Member Author

Closing in favor of #12711

@Youssef1313 Youssef1313 deleted the dev/ygerges/solution-gen branch October 31, 2025 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants