Use consistent field names #4387
Conversation
manfred-brands
force-pushed
the
UseConsistentFieldNames
branch
from
c95d159 to
47626fd
Compare
|
@nunit/framework-team any chance for a review. It is mainly text substitution. |
There was a problem hiding this comment.
Thanks @manfred-brands
I've been keeping an eye on this one but hadn't explicitly reviewed since I'd already weighed in on the original issue and wanted to let others have a chance.
Time got away from me, I hadn't realized it had already been a few weeks since you'd made this. Thanks again for putting this up, LGTM.
| @@ -61,52 +63,45 @@ dotnet_style_predefined_type_for_member_access = true:suggestion | |||
|
|
|||
| dotnet_naming_style.pascal_case.capitalization = pascal_case | |||
|
|
|||
| # Required | |||
| dotnet_naming_symbols.namespaces_types_and_non_field_members.applicable_kinds = namespace, class, struct, enum, interface, delegate, type_parameter, method, property, event | |||
| dotnet_naming_rule.namespaces_types_and_non_field_members.severity = error | |||
There was a problem hiding this comment.
@manfred-brands What are your thoughts on relaxing these formatting rules to warnings in the IDE? That way contributors can experiment with different approaches without worrying about formatting, but we could still have CI promote these to errors to prevent things from merging.
It looks like this was something we tried in the past here: https://github.com/nunit/nunit/blob/master/src/NUnitFramework/Directory.Build.props#L16-L19
There was a problem hiding this comment.
@stevenaw They are warnings:
Building with that error inside VS results in a successful build:
1>Done building project "nunit.framework.csproj".
1>D:\Development\3rd\NUnit\nunit\src\NUnitFramework\framework\Api\NUnitTestAssemblyRunner.cs(38,28,38,32): warning IDE1006: Naming rule violation: Missing prefix: '_'
1>nunit.framework -> D:\Development\3rd\NUnit\nunit\src\NUnitFramework\framework\bin\Debug\netstandard2.0\nunit.framework.dll
1>Done building project "nunit.framework.csproj".
========== Build: 1 succeeded, 0 failed, 0 up-to-date, 0 skipped ==========
The severity field in these don't seem to make any difference, only the setting of the 1006 severity does:
# IDE1006: Naming Styles
dotnet_diagnostic.IDE1006.severity = warning
I set the severity in the naming rules to error simply to match the existing enforced items.
There was a problem hiding this comment.
Now to how I feel about treating these as warnings inside VS.
Yes it might help a little, but when I see a warning when typing, I tend to fix it on the fly, even before building.
However, assume now we have a contributor that develops and tests in VS and ignores warnings.
As soon as the PR is pushed, it fails compilation. Same if they had run the build on the command line.
I think that is more frustrating than having to fix it "on the go".
But as I seem to be in the minority, I'm ok with leaving the BuildingInsideVisualStudio exception in place.
There was a problem hiding this comment.
Thanks @manfred-brands ! Perhaps it's something specific to my setup then, but I recently noticed IDE1005 (unused using) was actually failing my build within VS. I'd thought it related to these but perhaps not.
FWIW regarding addressing warnings, my personal flow and preference is also to remove warnings instead of ignore them. I usually just tend to do it at the end, between finishing a feature and pushing.
|
I'm going to go ahead and merge this since there don't seem to be any objections to the styles. Thanks once again @manfred-brands |
Fixes #4380
I also added a separate commit to enforce braces on multiline if/for statements but leave them optional on single line.