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 unnecessary CA2022 suppressions #47904

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Winniexu01
Copy link
Member

@Winniexu01 Winniexu01 commented Mar 26, 2025

Related to dotnet/source-build#4322

Remove all the unnecessary CA2022 suppressions

VMR: https://dev.azure.com/dnceng/internal/_build/results?buildId=2673846&view=results (internal Microsoft link)

@Copilot Copilot bot review requested due to automatic review settings March 26, 2025 08:51
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Request triage from a team member labels Mar 26, 2025
Copy link
Contributor

@Copilot 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 pull request removes the unnecessary CA2022 warning suppressions from test files to clean up the code. The changes update the test files by removing obsolete #pragma warning disable/restore statements.

  • Removed CA2022 suppression in GivenDotnetSlnAdd.cs
  • Removed CA2022 suppression in GivenDotnetSlnRemove.cs

Reviewed Changes

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

File Description
test/dotnet-sln.Tests/GivenDotnetSlnAdd.cs Removed obsolete CA2022 suppression lines
test/dotnet-sln.Tests/GivenDotnetSlnRemove.cs Removed obsolete CA2022 suppression lines

@Winniexu01 Winniexu01 marked this pull request as draft March 26, 2025 08:59
@KalleOlaviNiemitalo
Copy link
Contributor

Those are instances of FileStream, rather than MemoryStream or UnmanagedMemoryStream, so the CA2022 warnings correspond to actual bugs in the tests. Do you intend to fix those, e.g. by making the tests call Stream.ReadExactly?

@Winniexu01
Copy link
Member Author

Yes, I'm trying to fix the CA 2022 warnings by calling Stream.ReadExactly.

@Winniexu01 Winniexu01 reopened this Mar 28, 2025
@Winniexu01 Winniexu01 marked this pull request as ready for review March 28, 2025 11:12
@Winniexu01 Winniexu01 requested a review from a team as a code owner March 28, 2025 11:12
@Winniexu01
Copy link
Member Author

Add @dotnet/source-build and @dotnet/product-construction as reviewers.

Comment on lines 174 to 178
#if NET
await Stream.ReadExactlyAsync(Array.Empty<byte>(), 0, 0, cancellationToken);
#else
await Stream.ReadAsync(Array.Empty<byte>(), 0, 0, cancellationToken);
#pragma warning restore CA2022
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using #if NET to choose between ReadExactlyAsync and ReadAsync, I think it would be better to conditionally define ReadExactlyAsync as an extension method.

Copy link
Member Author

Choose a reason for hiding this comment

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

Update: 57ed326

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.

2 participants