Fix CA2000, CA2237, CA3075

[NicholasNoise]

Fix CA2000, CA2237, CA3075

Also requires #66 otherwise nuget restore fails.

[NicholasNoise]

@fluffynuts

[NicholasNoise]

I think we should refactor this somehow.

[fluffynuts]

@NicholasNoise I'm all for trying to up code quality; this adds a ton of noise though. For me, warnings are a thing that should not be seen -- all warnings should be attended to, either by solving the problem the warning is about or by intentionally suppressing the warning. Perhaps FxCop isn't the way to go? Perhaps we should look at something like SonarQube? It's equally noisy, but not in the build.
The problem with a sea of warnings is that no-one will deal with them, and they drown out potentially problematic stuff. For instance, you've fixed up a place where a StreamWriter wasn't disposed (good!), but how would I spot that in all of this if you hadn't? I really want all the fixes for stuff which hasn't been disposed, but I'm a little overwhelmed by the new warning noise.

Also, FYI, npm run build fails:

Util\PropertiesDictionary.cs(39,22): error CA2237: Add [Serializable] to PropertiesDictionary as this type implements ISerializable [C:\code\opensource\log4net\src\log4net\log4net.csproj]

Took me a while to find it in the swathes of warnings.

[NicholasNoise]

@fluffynuts Every warning you see may be (or should be) turned off by ruleset: some warning are essential and should be attended to, others are not and just ignore them with no regret.
FxCop grants build validation, so no one will be able to push "bad" code. (FYI: FxCop is build-in tool in dotnet 5) Standalone tools like SonarQube are good, but runs manually, periodically or by event-based triggers and generated results should be analyzed by someone. Once I saw generated report - like looking into spam or notification category in my mail box.

Anyway you are right that pushing a warning-free project into the dirt is no good.
I think to split this PR into 2 parts:

• Fix CA2000, CA2237, CA3075 - this pr
• Add FxCop - new pull request with ruleset and csproj configuration changes.
  Are you ok with it?

[fluffynuts]

That sounds like a good plan 👍

[fluffynuts]

btw, I don't have anything against FxCop, and I expect rules would have to be established, just like for SonarQube; I guess I'm just remembering how successful SonarQube was for me on one of my projects because I could chip away at warnings one by one until I got a clean bill of health without interfering with build elsewhere. I don't know enough about FxCop to be sure, but I'd expect that we could perhaps enable rule processing for a specific build configuration only -- and have that build configuration targeted by npm test, which can be run by CI (I have it running against my own appveyer, but haven't yet set it up for this official repository -- yet another Thing To Get Done).

Don't be afraid of an external tool though -- it can always be made part of the build pipeline. That's not a bump for or against any particular tool -- just a note.

[NicholasNoise]

That sounds like a good plan 👍

Done!

I don't know enough about FxCop to be sure, but I'd expect that we could perhaps enable rule processing for a specific build configuration only -- and have that build configuration targeted by npm test, which can be run by CI (I have it running against my own appveyer, but haven't yet set it up for this official repository -- yet another Thing To Get Done).

Two ruleset is the way to get things done (:

Don't be afraid of an external tool though -- it can always be made part of the build pipeline. That's not a bump for or against any particular tool -- just a note.

Build-in tools are preferable, an external tool makes it harder for new contributor to join. But in the same time used tools should be effective by all means.
I'm not familiar with SonarQube to make any conclusion so it is not up to me :)

[fluffynuts]

Build-in tools are preferable, an external tool makes it harder for new contributor to join. But in the same time used tools should be effective by all means.
I'm not familiar with SonarQube to make any conclusion so it is not up to me :)

I'm not talking about expecting people to install stuff -- external tools which can be bootstrapped by the build pipeline are ok -- for instance, running tests: for the primary run, we use the nunit runner, but how that happens is all tucked away in the bowels of zarro test-dotnet, which will download the nunit console runner nuget package & use it to run tests. Same goes for coverage via dotCover or OpenCover (and subsequent report generation with ReportGenerator, if using OpenCover). Where I can't just automate it, I try to provide helpers like the .ps1 scripts in the root of the repo to get more esoteric build targets installed. But for a new dev, if one has node & the relevant .net tools installed (eg vs build tools install), then

npm ci
npm release

will

• bootstrap all required tooling
• restore packages
• build
• test
• generate build artifacts

At some point, I want to automate the site into that (I have a build-site npm script which I use, just not 100% convinced it should be part of the build pipeline yet because release can be run at a CI server which wouldn't (shouldn't) be able to push back the generated site). Also need to fix up the sdk docs which went missing, and then that should be a part of the build-site pipeline.

The point is: I don't mind external stuff which can be automated. I try to reduce non-automatable dependencies, which should be documented (yes, I have work to do there too, in cleaning up the docs).

[fluffynuts]

@NicholasNoise I need to ask you for one more change before merging: the style of the project has been to use spaces for indentation. I noticed some whitespace changes on the prior PRs and just left it, but I'd really appreciate it if you could use spaces instead of tabs in this project. Perhaps I should set up an .editorconfig to assist with this...

[NicholasNoise]

the style of the project has been to use spaces for indentation.

One more reason to use something like StyleCop to prevent from happening.
Actually the style of log4net is tab-based indent for 90% of files.
But if you say so I can convert changed files.

[fluffynuts]

hm, I don't mind, really, what style is adopted -- in the file where I saw whitespace changes, it looked like the original code was spaces and your changes were tabs. Now I'd like to get an overview: whilst my preference is for spaces in C# (it's the default style, and I'm about adopting default styles instead of bucking them), I understand how tabs can be beneficial to the few people who care to set up configuration for them (:

StyleCop isn't enough -- a contributor may use something other than Visual Studio or a ReSharper-aware IDE (personally, I use Rider, but there's a case for VSCode, or whatever someone wants to use) to edit code. Most decent editors should understand .editorconfig and assist the user to maintain correct style without them having to respond to warnings. I'm sure StyleCop can do more too, so perhaps it could be used in conjunction with an .editorconfig. There are a lot of conventions which can be configured via .editorconfig, in addition to simpler things like indentation: https://docs.microsoft.com/en-us/visualstudio/ide/editorconfig-code-style-settings-reference?view=vs-2019

I think I'll just accept what you have here (because the fixes are good) and mull over (a) what the style should be and (b) how to help to enforce it (:

[NicholasNoise]

I prefer spaces too by its looks-the-same-everywhere feature.

StyleCop isn't enough

StyleCop and FxCop grant a build validation with right ruleset (SA1027 set to Error), so it is IDE independent.
IDE provides valid underlining for an incorrect tabulation. It is my favorite way to force things right.

I think I'll just accept what you have here

Thanks!

[NicholasNoise]

@fluffynuts

Is 2.0.11 planned to release any time soon?

[fluffynuts]

I've started a vote, have one +1, so waiting on 2 more.

[fluffynuts]

StyleCop isn't enough

StyleCop and FxCop grant a build validation with right ruleset (SA1027 set to Error), so it is IDE independent.

What I mean is that StyleCop is a "you broke it, so you need to fix it" solution -- a gate, if you will, which is good, but better is something which informs the editor of the correct choice to make so that when the coder presses , the right thing is done and the coder doesn't have to retroactively fix things or update their IDE prefs to fit in with StyleCop. EditorConfig provides this.

IOW, StyleCop is retroactive where editorconfig is proactive. Keep the retroactive to enforce style, bit provide the proactive to make it not the problem of the new contributor to get it right (:

Editorconfig was enough to get my team all on the same page across win/osx for another project - no errors or warnings, just editors fixing styles as people worked.

Anyway, it's all academic until I get some time to add a .editorconfig. I would welcome a StyleCop PR though - I don't use it, so wouldn't know offhand how to set it up. My opinion is that we should be helping contributors to make meaningful contributions with as little resistance as possible.

[fluffynuts]

@NicholasNoise 2.0.11 is out with these changes. Things take a little while because:

1. I hadn't had my gpg key accepted into the apache KEYS file yet
2. The process involves a vote, all from people who are doing this in their own time
3. I'm basically the only committer to this project, for now. Of course, I'm interested in helping other people to join me if they want to (:

[fluffynuts]

@NicholasNoise if you're interested in (3), let me know how to contact you. Your GitHub profile has no contact info ):

[NicholasNoise]

2.0.11 is out with these changes.

Thanks!

Things take a little while

There is no problem at all, I've saw insides of how-to-release-project - is not that simple as it looks.

@fluffynuts
I contribute open source projects on a non-regular basic, but for now.
Anyway you can contact me by mailing me at gmail.com (:

[fluffynuts]

There is no problem at all, I've saw insides of how-to-release-project - is not that simple as it looks.

yeah, I need to update that too, because quite a bit of helpful contributor documentation is out of date ):