Skip to content

Consolidate file-level directive manipulation#53136

Merged
jjonescz merged 7 commits intodotnet:release/10.0.3xxfrom
jjonescz:sprint-convert-editor
Feb 26, 2026
Merged

Consolidate file-level directive manipulation#53136
jjonescz merged 7 commits intodotnet:release/10.0.3xxfrom
jjonescz:sprint-convert-editor

Conversation

@jjonescz
Copy link
Member

@jjonescz jjonescz commented Feb 24, 2026

dotnet project convert used custom logic to remove #: directives which didn't handle whitespace as nicely as the FileBasedAppSourceEditor (used by dotnet package add/remove). This makes both commands use the same machinery.

Furthermore, fixes some bugs in whitespace handling (discovered by the directive tests that now use the new machinery).

@jjonescz jjonescz added the Area-run-file Items related to the "dotnet run <file>" effort label Feb 24, 2026
}
}

public static SourceFile RemoveDirectivesFromFile(ImmutableArray<CSharpDirective> directives, SourceFile sourceFile)
Copy link
Member Author

@jjonescz jjonescz Feb 24, 2026

Choose a reason for hiding this comment

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

Moved these to VirtualProjectBuildingCommand because

  • they now need to use the FileBasedAppSourceEditor which isn't available in this project,
  • there is no need for these APIs to live in this shared project since they are not used by anyone but dotnet CLI (@tmat I assume you don't need these in dotnet-watch?).

@jjonescz jjonescz marked this pull request as ready for review February 25, 2026 13:12
@jjonescz jjonescz requested review from a team and Copilot February 25, 2026 13:12
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 consolidates file-level directive (#:) manipulation logic by making both dotnet project convert and dotnet package add/remove use the same FileBasedAppSourceEditor machinery. The consolidation eliminates duplicate code and improves whitespace handling consistency across commands.

Changes:

  • Moved directive removal logic from VirtualProjectBuilder to VirtualProjectBuildingCommand, leveraging FileBasedAppSourceEditor for consistent behavior
  • Refactored WhiteSpaceInfo struct to distinguish between blank lines (BlankLineLength) and same-line whitespace (RestLength), enabling smarter whitespace removal
  • Fixed whitespace handling bugs where leading/trailing blank lines weren't removed correctly in certain scenarios

Reviewed changes

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

Show a summary per file
File Description
src/Cli/dotnet/Commands/Run/FileBasedAppSourceEditor.cs Updated Remove method to use new BlankLineLength/RestLength properties for more precise blank line removal
src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs Added RemoveDirectivesFromFile methods that use FileBasedAppSourceEditor for directive removal
src/Cli/dotnet/Commands/Project/Convert/ProjectConvertCommand.cs Updated to call VirtualProjectBuildingCommand.RemoveDirectivesFromFile instead of VirtualProjectBuilder.RemoveDirectivesFromFile
src/Cli/Microsoft.DotNet.FileBasedPrograms/FileLevelDirectiveHelpers.cs Refactored WhiteSpaceInfo to track blank lines separately from partial-line whitespace; updated whitespace calculation to exclude the directive's own span
src/Microsoft.DotNet.ProjectTools/VirtualProjectBuilder.cs Removed RemoveDirectivesFromFile methods (consolidated into VirtualProjectBuildingCommand)
test/dotnet.Tests/CommandTests/Run/FileBasedAppSourceEditorTests.cs Added tests for leading whitespace handling and whitespace outside lines
test/dotnet.Tests/CommandTests/Project/Convert/DotnetProjectConvertTests.cs Updated test expectation and method call to reflect new whitespace handling behavior
src/Cli/Microsoft.DotNet.FileBasedPrograms/InternalAPI.Unshipped.txt Updated API surface to reflect WhiteSpaceInfo property renames

@jjonescz jjonescz requested a review from RikkiGibson February 26, 2026 10:25
public required WhiteSpaceInfo LeadingWhiteSpace { get; init; }

/// <summary>
/// Additional trailing whitespace not included in <see cref="Span"/>.
Copy link
Member

Choose a reason for hiding this comment

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

checking my understanding: #: directives cannot have trailing // comments or similar? Is that right?

Copy link
Member Author

Choose a reason for hiding this comment

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

that's right, they cannot, everything on the same line as the directive is just its "content"

@jjonescz jjonescz merged commit bdb5a2a into dotnet:release/10.0.3xx Feb 26, 2026
33 checks passed
@jjonescz jjonescz deleted the sprint-convert-editor branch February 26, 2026 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-run-file Items related to the "dotnet run <file>" effort

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants