Iot: Enable style checks during the build

[krwq]

We should enable at least some style checks during the build.

This will avoid people to have to come back and fort with the PR iterations as they can be spotted by the build.

• [x] ensure src/devices/Directory.Build.props commented out code for style cop checking is re-enabled (added as part of one of the style cop PRs)
• [x] ensure we use spaces everywhere (we use tabs in some files right now and StyleCop doesn't complain). See
• [ ] re-iterate on all SuppressMessage

cc: @joperezr

[pgrawehr]

@krwq: I would like to pick this up and enable it at least for core. (I'll see how far I get). I'll try to come up with a reasonable rule set which requires only minimal changes (and no api changes of course) for now.

Some default rules we obviously break (such as SA1309 - Field names should not begin with underscores - where we go for SX1309 instead, which is just the opposite), but for some I'm not sure.

• SA1311 Static readonly fields should begin with a capital letter: We now have a mixture of s_ prefix and normal CamelCasing as for properties. I'd personally prefer the later (so enabling the rule) but what would you suggest? - I think the s_ prefix should be for static fields (not readonly).
• may come up with more

[krwq]

@pgrawehr yes, you can go ahead with that. Please split rules into 3 parts (add a comment or something so we can pick them up whenever someone feels like doing the work):

• rules we can enable right now with minimal effort
• rules we eventually want or not sure if they require setting up guidelines for the repo (please write a list in a new issue or PR description)

also you can assume at minimum we want what corefx has:
https://github.com/dotnet/runtime/blob/master/eng/CodeAnalysis.ruleset

and you can also see how they have enabled it

[pgrawehr]

A solution with all the projects can be created automatically using:

dotnet new sln -n Allprojects
for /R %r in (*.csproj) do dotnet sln AllProjects.sln add %r

Some further modifications may be required to configure the correct build target afterwards.

[pgrawehr]

With PRs #892, #894, #895, #897, #898, #900, #901, #902, #903 and #904, a basic set of style cop rules was defined and the files fixed (with some exceptions due to potentially breaking API changes).

The rules enabled in https://github.com/dotnet/runtime/blob/master/eng/CodeAnalysis.ruleset are also enabled here (plus some more), but with a few exceptions (note that the ruleset there only mentions the rules explicitly disabled):

• SA1121 Use Built-In type aliases. [I would leave this disabled for this project, as sometimes it's more clear to write i.e. Int16 instead of short when interfacing with hardware]
• SA1129 Should not use value type default .ctors. [Same here, low-level structures usually are not declared with constructors, and whether I write var a = new SomeStruct() or var a = default(SomeStruct) (the later being allowed by the rule) doesn't really make a difference.
• SA1217 Using statements should be ordered by alphabet. [No real preference on this one]
• SA1411 Attribute ctor must not use unnecessary parameters. [Could be done IMHO]
• SA1412 Files should be stored as UTF8. [Needs testing whether that's not already the case anyway, some files may heavily depend on that, see my PR #859 ]
• SA1609 Property documentation should have value. [Actually never seen this being used...]

In addition SX1101 "Do not prefix local members with this" should be enabled as well.

[krwq]

@pgrawehr I think you've already fixed all tabs, right?

[pgrawehr]

Yes, that should be fine. And it's also validated now (I think I even tested that the rule works).

[krwq]

Let me know if we should close this or if you're planning some more work here. For SuppressMessage we can create a separate issue on another occasion

[pgrawehr]

I will do a quick check whether any of the rules mentioned above can be enabled without lots of side effects, but after that, I think we can close it and focus on content again for a while. I guess we should for instance come to a conclusion on the "board" discussion.

[pgrawehr]

But it's good this is done now before the holidays, so if people find time to contribute now, they won't be pushed back by lots of merge conflicts.

[krwq]

@pgrawehr I feel like Board is going to be a longer design conversation :smile: Agreed on holidays although on the other hand it's vacation season for us and I'm soon gonna be on parental leave (I'll likely still be a bit active)

[pgrawehr]

I see. This could cause a bit of a PR backlog then, if you are off and @joperezr is the only one left reviewing. But we'll see. This is vacation season here as well (most offices are closed between xmas and new year), that's why I feel people might have time to contribute.

[krwq]

@pgrawehr I'll likely still review some PRs when I got some spare time but can't promise anything..

[Ellerbach]

@krwq what is left from this issue? I feel now all is reinforced, at least when I'm coding, I can clearly see Stylecop not being happy with tabs, extra spaces and other missing comments :-)

[pgrawehr]

The last point on the OP should probably be checked once. I'll give it a go next week.