Implement the 'elementOrder' configuration property

[sharwell]

Fixes #1745

❗ This pull request merges SA1214 and SA1215 into a single rule (SA1214). The Known Changes documentation and the documentation for each of these rules was updated to reflect the change.

💭 Additional tests for non-standard configurations will be required. The current tests cover a couple of common configurations (including extensive testing of the default configuration). I would prefer to get the new implementation out earlier for testing in the wild and add the additional tests separately.

[codecov-io]

Current coverage is 79.35%

Merging #1812 into master will decrease coverage by -0.10% as of a054a6c

@@            master   #1812   diff @@
======================================
  Files          556     554     -2
  Stmts        33700   33869   +169
  Branches      9305    9334    +29
  Methods                          
======================================
+ Hit          26778   26877    +99
+ Partial       5414    5412     -2
- Missed        1508    1580    +72

Review entire Coverage Diff as of a054a6c

Powered by Codecov. Updated on successful CI builds.

[vweijsters]

I had a quick look at the documentation only for now.

Overall it looks good. I got one remark:

• The documentation is incomplete with respect to describing what order is implied by the kind and accessibility settings. It can be reconstructed by looking at the various impacted rules, but it would be nice to have it all in the same place.

[sharwell]

@vweijsters I updated this to improve documentation and messaging.

[oatkins]

This seems like a step in the right direction to me. Legacy SC defines many ordering rules, but it really isn't useful to pick and choose which ones to enable because they all have implicit knowledge of each other. Amalgamating some of the rules is therefore very reasonable.

I'm not sure that I'll use the config option to customize the traits that determine the correct ordering, but I do appreciate that this makes the rules much more explicit and easier to understand. We are also moving away from the legacy "my way or the highway" approach; a lot of people will appreciate that.

Looking at this on my phone, so I've not studied the code; just the documentation. But a wholehearted +1 on that basis.

[sharwell]

@oatkins I believe we will have the ability to expand on this customization in the future to cover the "kind" and "accessibility" traits in more detail. But I agree, the most important aspect right at this point is clarity regarding the relative order of the rules.

I'll wait for @vweijsters to finish the implementation review before merging.

[vweijsters]

I think that SyntaxKind.EventDeclaration should also be addressed, as events can also be implement explicitly.

[sharwell]

➡️ Can you file this as a separate bug? For this PR I just reused the code from #1180/#1179. So far it hasn't been reported as a deviation from StyleCop Classic so I'd like to get a bit more discussion to ensure the change is intentional and properly documented.

[vweijsters]

Done -> #1823

[vweijsters]

Finished my review (as you may already have noticed 😉). There's several issues that should be addressed, but the overall design looks good to me.

@sharwell Big kudos for keeping this well readable / well understandable.

[vweijsters]

👍 Looks good now

[martincostello]

@sharwell I've not reviewed the code, but it turns out I accidentally functionally reviewed this earlier by updating a repo of mine to beta017 and using the new rules that were added.

Specifically, the rule found two cases where I have a constant after a field, but I decided to change the order configuration from the default instead instead and leave the code (1, 2) as-is.

[sharwell]

@martincostello The configuration in your example is the default.

[martincostello]

@sharwell Turns out I misread Configuration.md and thought that the first JSON example was what the default was, rather than a tweaked version, so I put what I thought was the default in explicitly and then changed that to do what I wanted, which actually was the default.

[sharwell]

It looks like you like to have your configuration be explicit. If that is the case, I recommend pulling your default configuration directly from the schema, which is actually pretty easy since you have a link to the schema as the first line of your configuration file. 😄

[martincostello]

@sharwell I had some caching issues in Visual Studio earlier on where it wouldn't pick up the updated intellisense for the new members, even after I explicitly opened it in the IDE from the schema URL, which is why I was working from the markdown rather than it. Sounds like a good idea for me to do though!

[sharwell]

@martincostello Open up your stylecop.json file and right click anywhere in the document. Then select Reload Schemas.

[martincostello]

Ah...thank you!

[sharwell]

That might be a good tip to add to EnableConfiguration.md if you want to. 😉 (This file is a minimal bit of usability information for first-time users of stylecop.json. We add a link to this file when we generate stylecop.json via a code fix.)

[martincostello]

Done - #1850