Fix CA2000 analyzer warnings #3824
Conversation
nkolev92
left a comment
nkolev92
left a comment
There was a problem hiding this comment.
I think we should try to make use of some modern using declaration pattern to reduce the footprint.
wyt?
kartheekp-ms
force-pushed
the
dev-kartheep-ms-ca2000-warnings
branch
from
6da4b65 to
9fa2ab1
Compare
nkolev92
left a comment
nkolev92
left a comment
There was a problem hiding this comment.
Just a question or two about what the analyzer fix is in that scenario.
I also left a few style nits.
|
|
||
| using (var package = new PackageArchiveReader(outputPath)) | ||
| { | ||
| using var package = new PackageArchiveReader(outputPath); |
There was a problem hiding this comment.
This seems like it's only changing from old usage to new usage. Is there an analyer fix here?
There was a problem hiding this comment.
Given that it's a stylistic fix, rather than a functional one, I'd normally say, avoid doing that to not introduce unnecessary changes. Especially when readability is not necessarily changing too much.
Given all that, I won't block on that, you can make a judgment call/discuss this feedback with others from the team.
There was a problem hiding this comment.
Given that it's a stylistic fix, rather than a functional one, I'd normally say, avoid doing that to not introduce unnecessary changes. Especially when readability is not necessarily changing too much.
Sure, I agree. I also feel setting Language Version to 8 to core libraries a bit restrictive because net5.0 already defaults to Language Version 9. Since all of our projects targets multiple TFMs including net472, I think we will be struck to C# 7.3 which is the default language version for all .NET framework projects.
I prefer reverting my last commit to this branch where we added '8` for few projects unless other team members has additional feedback.
There was a problem hiding this comment.
I'm not suggesting going back to 7.3 actually.
I'm merely suggesting address only CA2000 warnings in this PR.
We should move to 8 or even 9 wherever we can.
There was a problem hiding this comment.
Got it. Thanks, Nikolche for clarification. AFAIK, I modified using statement while fixing CA2000 warnings in this PR but not in other places. I will fix the nit pics suggested in other comments in the next commit.
src/NuGet.Core/NuGet.PackageManagement/FileModifiers/XmlTransformer.cs
Outdated
Show resolved
Hide resolved
nkolev92
left a comment
nkolev92
left a comment
There was a problem hiding this comment.
Nice job ;)
Make sure you read my comment in the previous thread :)
kartheekp-ms
force-pushed
the
dev-kartheep-ms-ca2000-warnings
branch
from
9fa2ab1 to
2299298
Compare
|
🔔 @NuGet/nuget-client |
|
Planning to merge this PR by end of day today. Happy to address additional feedback. |
Bug
Fixes: https://github.com/NuGet/Client.Engineering/issues/643
Regression: No
Fix
Details: Fixed CA2000 analyzer warnings in the product code. 36 out of 90 violations were fixed and added
justificationinGlobalSuppressions.csfor the remaining violations.Testing/Validation
Tests Added: No
Reason for not adding tests:
Validation: All checks passed in CI