Consistently use _ prefix for field names

[manfred-brands]

Our current .editorconfig says that private fields should be prefixed with an _ and in camel case.

Our code base does not consistently reflect this.
For maintainers a consistent code style is much easier to work with.
Having a prefix for fields makes it obvious in code reviews if code uses a field or a local variable.

Increase level of IDE1006 Naming rule violation to warning and fix resulting violations.
At the same time fix this.name = name where the this prefix was needed because fields and parameters were named the same.

[manfred-brands]

Waiting for #4377 to be merged to prevent likely conflicts.

[SeanKilleen]

@manfred-brands as part of the PR to fix this, we may want to run dotnet format --fix-analyzers --diagnostics IDE1006. I think that will have the effect of making it consistent across the codebase.

We may also want to look into a PR that runs dotnet format against the code, which respects the rules in the .editorconfig file. After that, we could use a GitHub action along the lines of https://github.com/marketplace/actions/dotnet-format to enforce that on a PR basis.

[OsirisTerje]

This is just me,(perhaps this a :-), but I actually don't like the underscore prefix, it is a remnant from Hungarian notation that still lingers on. I don't think it improves readability much, and notation like this.name = name doesn't bother me when reading the code.
I do understand the issue with mixing local variables with class fields, but that is more of a problem when the class is big and the methods long. That is a big issue to me, and we have way too much of that.
Also, complicated logic, unclear layering, strange naming in general, really slows down debugging when tracking down issues.

And if I really need to keep track of mixed fields/variables, a field is easily made into a private property, and the issue is gone.

Also, whenever we do large formatting or other cleanup of the code, all code is marked as being touched by that person, and I know that I at least have asked people about troublesome code that has their name, just to find out they are not responsible for the actual code, they have only reformatted it.

So, that said, please feel free to reformat, but please let it be fast :-)

[lahma]

As an outside contributor I'm willing to choose either _ like MS uses or none (Quartz.NET has no prefix, for now). In my opinion the key for each codebase is helpers like IDE giving a warning or even an error or good documentation to comply with the general coding style in the repo.

I used to tell people I guided towards repositories that changing formatting or conventions is all fine as long as the team is OK and you are willing to ensure the proposed convention is being followed (key point here is not having partly enforced conventions and codebase diverged to different directions).

[manfred-brands]

@SeanKilleen The idea of the real-time analyzer checks is that violations are shown when editing and then easy corrected. We have used this successfully at my work. That way following the coding standard becomes 2nd nature. Then again, this is only the case if using VS. Not if using editors like 'vi' (or even VS code).
If the analyzer is configured severely enough, violations will fail the local compilation and developers would still need to fix them before pushing. Alternatively we can set the level to suggestions, to not fail the build and use the dotnet format action automatically applied to PRs. For PRs that are already compliant, no format commit will be added, for those that aren't a commit is automatically added. So that might be the best of both worlds.

[manfred-brands]

@OsirisTerje

I actually don't like the underscore prefix, it is a remnant from Hungarian notation that still lingers on. I don't think it improves readability much, and notation like this.name = name doesn't bother me when reading the code.

I only mentioned the _ because that is the current .editorconfig configuration in the repository since 2017.
We just don't follow it and as it isn't enforced things diverge and become inconsistent.
We do enforce pascal_case for types, methods, properties and camel_case for parameters and locals, just not for fields.
Maybe @jnm2 remembers the reasons for that at the time.
Was it to prevent a big formatting commit?

[stevenaw]

FWIW, I think VS Code supports Roslyn analyzers and possibly fixers via the Omnisharp extension, though I don't have experience with it myself (https://www.strathweb.com/2019/04/roslyn-analyzers-in-code-fixes-in-omnisharp-and-vs-code/).

I understand @OsirisTerje 's feeling about file history and this change, unlike the "explicit visibility" change, cascades to each line of usage. I also think that if we wanted to establish conventions we should use tooling to help nudge us and others towards it if it can be done unobtrusively. Though I'd also suggest something like that might be good to get a broad team acceptance of, or at least we should be sure no one is strongly against the proposed conventions.

All that is to say, I don't have a strong opinion on this particular convention myself so I would be fine with whatever others decide.

FWIW, it looks like the team had a bit of a previous discussion about this issue in #2625. It seems to have ended due to inactivity. It looks like @rprouse and @jnm2 each had some good thoughts there. I'm unsure if any opinions have since changed

[OsirisTerje]

Thanks @stevenaw for pointing to #2625, I wasn't aware of that discussion, but I seem to be in the same camp as Charlie and @rprouse .

I also looked at some of my [better] code pieces and notices that I actually don't use local variables so much at all, so there are not many clashes.
If the class is small and the methods likewise, you often don't need any local variables, and then you just have some parameters and when the method is small, you clearly see which is which anyway.
But once the method goes beyond (let's say > 10-20 lines) then it starts to get messy.
And in the NUnit code base, we do have a lot of that, unfortunately.

@manfred-brands have done an awesome job with the nullability stuff, and there he touched nearly the whole code base, and others have done some minor refactorings that also have touched a ton of files, so if this is done too, there is not much extra loss anyway. So, IMHO, whatever is good enough for me.

[jnm2]

My thoughts are: editorconfig should be used to avoid PR requests for changes. Whatever we decide to ask for in PRs, that should be advertised up front using warning squiggles where l when possible. Ideally I would also correct any warnings in existing code then too so that the error list is useful and new warnings aren't lost amid a sea of dozens of existing warnings.

I think there's a lot to be said for warnings only, then PR validation setting WarningsAsErrors. That way you can quickly iterate and run and debug tests locally without polish, then commit/submit PR with polish.

[manfred-brands]

I think there's a lot to be said for warnings only, then PR validation setting WarningsAsErrors. That way you can quickly iterate and run and debug tests locally without polish, then commit/submit PR with polish.

If the warnings are shown at edit time, it becomes second nature to fix them and use the correct naming/style.
If it runs locally, they going to push the PR and which then will fail.

[jnm2]

The thing is, it's so significantly frustrating to have to polish everything before even running and debugging- subjectively, I'd rather err on the side of trusting contributors and letting them figure out PR build failures than annoy them by putting avoidable friction in their way. That's how I'd want to interact in other projects as a contributor. But, I don't feel so strongly that I'd hold up a more active NUnit maintainer who has a different preference for interacting with contributors.

Showing instant non-blocking warnings which become errors on push has seemed like a sweet spot in places I've been.

[stevenaw]

Hmm. Given @jnm2 's comment about frustration, I wonder if surfacing these as errors in the IDE rather than warnings could cause extra friction for new contributors. There's already enough struggle learning to build and debug a new codebase that we may want to avoid adding more

[jnm2]

Does NUnit already have TreatWarningsAsErrors set in CI builds? I forgot to clarify that this is possible and in my experience has been a good balance.