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

Fix CA1000/S4023/S3442/S107/SA1402/SA1649 warnings #2089

Merged
merged 29 commits into from
May 1, 2024

Conversation

iamdmitrij
Copy link
Contributor

@iamdmitrij iamdmitrij commented Apr 30, 2024

Pull Request

The issue or feature being addressed

#1290

  • Enable CA1000 check only for non-public types
  • Make S4023 a suggestion
  • Fix S3442 by making abstract classes constructors private
  • Set S107 max param count to 12
  • Disable SA1402 because this repo doesn't follow such file/class structure
  • Fix SA1649 by consolidating class/file names. As far as I can tell it doesn't break any public APIs, just renaming C# source files
  • Add eng/analyzers configs to solution file in Solution Items

Details on the issue fix or feature implementation

Confirm the following

  • I started this PR by branching from the head of the default branch
  • I have targeted the PR to merge into the default branch
  • I have included unit tests for the issue/feature
  • I have successfully run a local build

Copy link

codecov bot commented Apr 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.72%. Comparing base (3c642fd) to head (a6acc1a).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2089   +/-   ##
=======================================
  Coverage   83.72%   83.72%           
=======================================
  Files         312      312           
  Lines        7126     7126           
  Branches     1054     1054           
=======================================
  Hits         5966     5966           
  Misses        789      789           
  Partials      371      371           
Flag Coverage Δ
linux 83.72% <100.00%> (?)
macos 83.72% <100.00%> (?)
windows 83.72% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@iamdmitrij iamdmitrij marked this pull request as ready for review April 30, 2024 20:03
@martincostello
Copy link
Member

Your PR has conflicts that need resolving.

Before starting a PR, ensure your fork is up to date and create a branch (don't work off main).

@iamdmitrij
Copy link
Contributor Author

@martincostello could you clarify what conflicts are we talking about? Merge conflicts? If so I don't see them on this PR

image

And my fork is up to the date

image

@martincostello
Copy link
Member

Ah, it's because the merge method was set to rebase:

image

The recommendation still stands though - if you do any further PRs, please sync your fork with our main branch and then create a new branch before starting new work.

@martincostello martincostello added this to the v8.4.0 milestone May 1, 2024
@martincostello martincostello merged commit a31bdc2 into App-vNext:main May 1, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants