Naming Errors

[CharliePoole]

I'm getting "Naming Errors" in my build of the framework. I don't know of any time when we all agreed that Visual Studio would be set up to do that. I assume it was done as a PR, which only requires two team members to review it, but I think stuff that constrains us all to work in a certain way requires either unanimity or a directive from the project lead. At a minimum, I think it requires discussion.

[mikkelbu]

My guess is that the "Naming Errors" are due to #2320 (but it is only a guess as I'm not quite what Naming Errors cover). As far as I can remember the rules defined in the editorconfig file should just be the standards from https://github.com/nunit/docs/wiki/Coding-Standards.

[CharliePoole]

Right. But up to now, as far as I was aware, the guidelines in editor config were not enforced in any way. In fact, that lack of enforcement is what convinced me it was OK to do. But I'm seeing errors right in the VS error window along with compile errors, although they don't seem to show up on every compile. I'd appreciate if somebody can explain how this happened.

My own view has always been that having rules enforced by software is an anti-pattern in any team.

[CharliePoole]

You're right about #2329, which fixed issue #2319.

The issue was originally written to enforce byte order marks. It seemed like a good idea and I for one didn't give it any further attention. Rob added a comment saying he wanted the coding standards to be enforced to the extent that they were in the editor config and I guess that was done.

In our governance structure, the project lead has the power to set rules like that. I have no qualms about it. He can decide whether to take advice from others or just make his own choices. At a minimum, however, I think the lead ought to announce the rules he is setting, not slip them in through a software fix.

It's that kind of slipstream promulgation of rules that makes me say automated rule enforcement is an antipattern in most teams.

I'm closing this issue as won't fix and chalking it up to experience.

[rprouse]

@CharliePoole you can hide the editorconfig warnings from the error window by switching the dropdown to build only

I do that for every legacy solution, VS2017 added way too much noise in intellisense warnings.

[CharliePoole]

When it lists it shows these things as errors, not warnings. I guess I'll wait to see if my CI builds can pass. Note that if the errors lead contributors to make wider changes in the source, they will be doing something we have always asked that they not do as part of a PR.

[rprouse]

I didn't realize they were showing up as errors, they should be warnings. Let's re-open this issue and modify accordingly.

[jnm2]

What's probably happening is that .editorconfig settings I added caused existing names that don't follow the coding convention to show up as errors. The thought was that it would highlight newly typed names that violated the coding standard as errors. I agree that they should not be errors unless we are willing to bring the codebase into alignment with the coding standard.

That's these three lines:

nunit/.editorconfig

Line 63 in 13b4776

dotnet_naming_rule.namespaces_types_and_non_field_members.severity = error

nunit/.editorconfig

Line 69 in 13b4776

dotnet_naming_rule.public_fields.severity = error

nunit/.editorconfig

Line 76 in 13b4776

dotnet_naming_rule.parameters_and_locals.severity = error

[jnm2]

By the way, any errors or warnings caused by the .editorconfig only show up when the editor window is open. The compiler remains unaware that they exist until this work is merged: https://github.com/dotnet/roslyn/projects/18

[CharliePoole]

I think I was seeing the errors on files I hadn't touched.

[jnm2]

Perhaps they are making changes. What I have seen is that the errors hang around for minutes after you close the editor window, even if it was only open for ten seconds and you didn't edit anything.

[jnm2]

I'll take this one. I may be able to simplify the naming settings, too. I lodged and subscribed to a great number of issues on .editorconfig last spring.

[jnm2]

I'm really tempted instead to bring the codebase in line with the naming standard. Would that bother anyone?

[CharliePoole]

It would have one good effect IMO - reviewing the PR would be a concrete way to review the naming standard. We have trouble, I think, getting good feedback when we talk in abstractions.

However, I'd still suggest several PRs divided by namespace for ease of review. Once the number of files to review gets past 50 or so there's no real review.

[jnm2]

Quoting #2625 (comment) by @rprouse:

I have two issues with a massive sweep update,

1. It with make it harder to track down changes to lines with git annotate because so many lines will be formatting changes
2. If we have any number of open PRs, it will likely cause merge conflicts and make their diffs harder.

Personally, I agree with @CharliePoole in that excessive coding standards are more of a pain than they are worth, especially whitespace before and after brackets. Certain things signal usage, like properties starting with a capital, but I don't see many violations of those standards.

The only hard rule is tabs vs spaces. Tabs are evil 😈

These are good reasons and I'm thinking they would apply here.