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

[WIP] Add FxCop to the build #449

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

Conversation

Numpsy
Copy link
Contributor

@Numpsy Numpsy commented Apr 16, 2020

Add FxCop to the build to see what issues it finds.

Marked as WIP because it creates over a thousand warnings in the build.

Some of them look worth looking at (e.g. warnings about not disposing of things before they go out of scope), but a lot of them are things like passing string literals to exceptions rather than using resources, which are possibly not really useful. (so could be turned off)

(Note: #445 is something that was found by the analyzer)

I certify that I own, and have sufficient rights to contribute, all source code and related material intended to be compiled or integrated with the source code for the SharpZipLib open source product (the "Contribution"). My Contribution is licensed under the MIT License.

@piksel
Copy link
Member

piksel commented Apr 17, 2020

Good initiative. I basically did this and turned it off again because of the spamming, but we should set up a ignore list, including CA1303 (until we get localization).

@Numpsy
Copy link
Contributor Author

Numpsy commented Apr 17, 2020

Is it ok to use the editorconfig approach to control the analyzer settings?
(It seems to be the MS recommended way now in VS 2019, but I can't say offhand if that works in 2017)

@Numpsy
Copy link
Contributor Author

Numpsy commented Apr 18, 2020

I take the way that the number of warnings in the CI build didn't go down after updating editorconfig to mean the answer to my previous question is 'no' :-(

In any case, I sent #450 to fix some of the produced warnings.

@Numpsy
Copy link
Contributor Author

Numpsy commented Aug 17, 2020

@piksel I left this using the 2.9 version of FxCop as the release notes for the v3 version say it requires VS 2019 and I thought that may not be wanted, but maybe that's not an issue now the tests are targetted at Core 3.1 and need that anyway?

@piksel
Copy link
Member

piksel commented Sep 8, 2020

Yeah, I don't see any problems with requiring VS 2019 for FxCop. If a need for this to change emerges then we could just disable or change this later, but for now I don't see any reason to use anything earlier than 2019.

@Numpsy
Copy link
Contributor Author

Numpsy commented Oct 4, 2020

hmm, I don't know how this change could cause that one test failure.

@piksel
Copy link
Member

piksel commented Oct 6, 2020

No, that's probably due to something unexpected in the CI. Especially since it worked on the same platform, different config, and same config, different platform(s).

@codecov
Copy link

codecov bot commented Apr 28, 2021

Codecov Report

Merging #449 (7418872) into master (34879be) will decrease coverage by 0.11%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #449      +/-   ##
==========================================
- Coverage   73.35%   73.23%   -0.12%     
==========================================
  Files          68       68              
  Lines        8721     8721              
==========================================
- Hits         6397     6387      -10     
- Misses       2324     2334      +10     
Impacted Files Coverage Δ
...ICSharpCode.SharpZipLib/BZip2/BZip2OutputStream.cs 78.40% <0.00%> (-1.13%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34879be...7418872. Read the comment docs.

@piksel
Copy link
Member

piksel commented May 11, 2021

I'm thinking about how to best get this merged. The best thing to do is probably to just create a bulk commit/PR with most of the fixes done. This will render other PRs in need of rebasing, but at least it would be more of an one-time event.

@Numpsy
Copy link
Contributor Author

Numpsy commented May 11, 2021

I think a pretty large proportion of the remaining warnings were about checking public params for being null, and at the time I might have thought that #501 would deal with some of those and not looked in much detail.
I'll have a look, and see what proportion are 'non-functionality' ones.

@Numpsy
Copy link
Contributor Author

Numpsy commented May 11, 2021

Theres also the question at this point on if it should use NetAnalyzers instead of FxCop (I can see about a PR that fixes many warnings at once in either case though))

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