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

[StyleCleanUp] Addressing SA warnings Part 3 #10125

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

harshit7962
Copy link
Member

@harshit7962 harshit7962 commented Nov 27, 2024

Description

This work is a part of our initiative to set code-style guidelines, align WPF and WinForms, and ensure PR standards with respect to styles. This in turn will help us in better maintainability and readability of the repo overall. The changes follow up from the PR #10080 and references to the issue #10017.

The current changes addresses the following Errors/Warnings in the src folder of WPF:

  • SA1400: Member should declare an access modifier

Customer Impact

None

Regression

None

Testing

Local Build Pass

Risk

Medium, could cause issues with reflection.

Microsoft Reviewers: Open in CodeFlow

@dotnet-policy-service dotnet-policy-service bot added the PR metadata: Label to tag PRs, to facilitate with triage label Nov 27, 2024
@harshit7962 harshit7962 marked this pull request as ready for review January 8, 2025 11:06
@harshit7962 harshit7962 requested review from a team as code owners January 8, 2025 11:06
@h3xds1nz
Copy link
Contributor

h3xds1nz commented Jan 8, 2025

@harshit7962 This feels kinda duplicated with #10021

@harshit7962
Copy link
Member Author

harshit7962 commented Jan 8, 2025

Yes, objective of both of the PRs is more or less the same. The only difference being that #10021 does it only for System.xaml while this PR addresses it for the src as a whole.

Also, I just realized that IDE0040 is probably the same as SA1400, haven't gone through the documentations thoroughly so cannot point out any subtle differences at the moment.

Having said that, I am okay with your PR being taken in first and then this one could go in to make up for the changes in shared folder.

Also, can you confirm if changing the severity here to warning result in a successful build of the repo? If yes, we can remove this line from the file.

@h3xds1nz
Copy link
Contributor

h3xds1nz commented Jan 8, 2025

@harshit7962 IDE0040 picks up more than SA1400 by a ton (50 vs 511 on my PR), its a bit newer as well.

My PR fixes the warning for System.Xaml only, but I will expand it for the entire solution in one PR, that's fine, it will yield like +5k fixes which will also include the SA1400 as those are reported (as IDE0040 just picks additional ones including the SA1400 ones).

So we couldn't remove the override for .editorconfig IDE0040 neither in mine or this PR at current state.

@harshit7962
Copy link
Member Author

Cool, will take only #10021 and await for the complete clean-up of IDE0040 (and hopefully SA1400 will be resolved as well) in a different PR. For now, re-marking this as a draft.

@harshit7962 harshit7962 marked this pull request as draft January 9, 2025 05:32
@harshit7962 harshit7962 marked this pull request as draft January 9, 2025 05:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
draft PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants