Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable Stylecop analysis #1037

Merged
merged 75 commits into from Jun 4, 2018
Merged

Enable Stylecop analysis #1037

merged 75 commits into from Jun 4, 2018

Conversation

blairconrad
Copy link
Contributor

@blairconrad blairconrad commented May 23, 2018

Closes #1035.

@blairconrad
Copy link
Contributor Author

blairconrad commented May 23, 2018

As you can see, I did not enable all the rules. The first step I took was to explicitly ignore in the AutoFixture.ruleset all the violations. This gave a clean build. Then I started enabling, to see what damage was done, and repair it. In general, I just went in ascending order by the rule identifier, except I did the "things that obviously cared about adding or deleting lines" first, because if you're going to remove a line, then at least you won't be bothered about its whitespace later.

Mostly I took a "let's do what StyleCop wants" approach, because I assume people put thought into the rules, and because it's easy (in theory) to just remove a commit from a PR if you don't like it. In some cases, it appeared that a rule violated your coding conventions (for examplle, "SA1633: The file header is missing or not located at the top of the file"), in which case I left the rule ignored in the ruleset.

Additional notes:

  1. In a very few cases, I suppressed rules in the source rather than fixing them or leaving them totally ignored. The commit messages tell you which.
  2. I only suppress "SA1515: Single-line comment should be preceded by blank line" in the tests, because most of the production code followed the rule, but the tests had a pervasive pattern of putting "// Act" and the like on the lines right after code. I don't mind adding the extra lines, but it seemed like something you didn't want.
  3. On that note, the ruleset file for the tests is called Empty.ruleset. I'm not sure it still counts as "empty", but I didn't feel it was up to me to change the name. I would, if you liked.
  4. In most cases, the "fix" I applied was the one that the analyzer suggested. In very rare cases, I deviated slightly, like where returning early from a function meant not having to worry about how the braces in the "then" portion of an "if" statement were formatted.

Rules I did not address (at this time):

  1. "SA0001: XML comment analysis is disabled due to project configuration". So many projects don't build documentation from the XML
  2. Member ordering rules. There were many violations, so I wasn't sure if it was something that's important to you. (Aside: when I first started working on FakeItEasy, the member ordering confused the heck out of me. But after a short time, I really came to enjoy it.). Also, even if you wanted the members reordered, I'd probably wait, because combining that with the other changes can make for very confusing diffs. I'd be happy to reorder in another set of commits and/or another PR
  3. "SA1413: Use trailing comma in multi-line initializers". This is polarizing, and I didn't have a sense of the prevailing state of the code. Happy to fix if you like.
  4. Parameters/parameter lists/elements spanning multiple lines (or being on one line, like if … return). Again, there were enough violations that it seemed you weren't bothered by it.
  5. Anything to do with missing documentation. There were a fair number, and adding the documentation is a lot of work, and perhaps not something best done by an outsider. That being said, I would, but like the "reording" comment above, would prefer to defer.

As always, I'm happy to discuss anything about the PR. Also, I'll revert to suppressing any rule you want, or start fixing the ones still suppressed, or fix or suppress something differently. Oh, and I realize you might not want 70 commits going into the history (but think of how I'd jump up the "contributions leaderboard"!), so I'm always happy to squash once we've a set you're happy with, either down to 1, or maybe one commit per rule category.

Thanks for considering the PR!

@zvirja
Copy link
Member

zvirja commented May 23, 2018

@blairconrad It's a fundamental PR, you have affected almost all the code we have 🤣

I'm going to dive into this in a day or two - just want to let you know that I'm not ignoring you 😅 This one requires a deep look, so it's a bit hard to find enough time in a row.

I noticed that you did MASSIVE work here and tuned a lot of weird stuff we have in the repo. Let me take a closer look and get back soon. Great thanks for your work so far! 😌

@blairconrad
Copy link
Contributor Author

blairconrad commented May 23, 2018

you have affected almost all the code we have

Oh? Which file did I miss? I'm sure there's something we can do to it. 😉

No rush or anything. And if you only have small amounts of time, you can always assess one rule (commit) at a time.

It's probably less work than you think. After all, StyleCop contains fixups as well as detection. It as (mostly) enable a rule, run the build, run the fixup, repeat. you can do it while watching TV!

Have fun!

blairconrad and others added 26 commits May 24, 2018 21:10
@zvirja
Copy link
Member

zvirja commented May 24, 2018

@blairconrad I had a chance today to review the list. For most of the cases I agree with the changes. In a few cases I disabled rule for the test projects (and reverted the change for them) as it brought more pain than benefit:

  • SA1401: Field should be private
  • SA1402: File may only contain a single type
  • SA1133: Each attribute should be placed in its own set of square brackets
  • SA1516: Elements should be separated by blank line

Additionally, I've rebased your branch on top of the latest master and updated StyleCop package to the latest version.

Oh, and I realize you might not want 70 commits going into the history (but think of how I'd jump up the "contributions leaderboard"!), so I'm always happy to squash once we've a set you're happy with, either down to 1, or maybe one commit per rule category.

Not that I don't want you to see in the top contributors list, but that's too easy way 🤣 A joke.
If seriously, I'll probably squash all the changes together. Last time I had a similar situation with MS code analysis rules related to patterns (ex FxCop). There I left all the commits as each fixup affected the code logic a bit, so I wanted to keep the reason. Here most of the changes are cosmetic, so I don't see too much benefit from preserving the commits.


I'm happy to proceed with how it looks now, however before merge I would like to change one more thing. I noticed that the added analyzers slowed down compilation and VS started to hang more 😟 Therefore, I would like to find a way to disable all the rules for the non-verification build and fail only for Verify configuration build. Do you now how to grab full list of the StyleCop rules, so I can suppress them all via a ruleset file? I tried <ImportAll Action="None" />, but it doesn't work unfortunately and I need to disable each rule individually 😢

I'll gladly do the work by myself - just provide me with the source of all the rule ID if you know 😉

Thanks a lot - I love the change! Thanks for suggesting the idea 😊

@blairconrad
Copy link
Contributor Author

updated StyleCop package to the latest version

Bah! I'd just grabbed the version from another package and forgot to check for updates. Thanks/sorry.

Do you now how to grab full list of the StyleCop rules

I don't know of a programmatic way, but from a quick survey of the .cs files using

 Get-ChildItem -Recurse -Filter *.cs | % { $_.Name } | Where-Object { $_ -match  "^[A-Z]+[0-9]{4}" } | Sort-Object

plus a little post-processing in emacs gave me this:

<Rule Id = "SA0001" Action = "None" /> <!--  Xml comment analysis disabled -->
<Rule Id = "SA0002" Action = "None" /> <!--  Invalid settings file -->
<Rule Id = "SA1000" Action = "None" /> <!--  Keywords must be spaced correctly -->
<Rule Id = "SA1001" Action = "None" /> <!--  Commas must be spaced correctly -->
<Rule Id = "SA1002" Action = "None" /> <!--  Semicolons must be spaced correctly -->
<Rule Id = "SA1003" Action = "None" /> <!--  Symbols must be spaced correctly -->
<Rule Id = "SA1004" Action = "None" /> <!--  Documentation lines must begin with single space -->
<Rule Id = "SA1005" Action = "None" /> <!--  Single line comments must begin with single space -->
<Rule Id = "SA1006" Action = "None" /> <!--  Preprocessor keywords must not be preceded by space -->
<Rule Id = "SA1007" Action = "None" /> <!--  Operator keyword must be followed by space -->
<Rule Id = "SA1008" Action = "None" /> <!--  Opening parenthesis must be spaced correctly -->
<Rule Id = "SA1009" Action = "None" /> <!--  Closing parenthesis must be spaced correctly -->
<Rule Id = "SA1010" Action = "None" /> <!--  Opening square brackets must be spaced correctly -->
<Rule Id = "SA1011" Action = "None" /> <!--  Closing square brackets must be spaced correctly -->
<Rule Id = "SA1012" Action = "None" /> <!--  Opening braces must be spaced correctly -->
<Rule Id = "SA1013" Action = "None" /> <!--  Closing braces must be spaced correctly -->
<Rule Id = "SA1014" Action = "None" /> <!--  Opening generic brackets must be spaced correctly -->
<Rule Id = "SA1015" Action = "None" /> <!--  Closing generic brackets must be spaced correctly -->
<Rule Id = "SA1016" Action = "None" /> <!--  Opening attribute brackets must be spaced correctly -->
<Rule Id = "SA1017" Action = "None" /> <!--  Closing attribute brackets must be spaced correctly -->
<Rule Id = "SA1018" Action = "None" /> <!--  Nullable type symbols must not be preceded by space -->
<Rule Id = "SA1019" Action = "None" /> <!--  Member access symbols must be spaced correctly -->
<Rule Id = "SA1020" Action = "None" /> <!--  Increment decrement symbols must be spaced correctly -->
<Rule Id = "SA1021" Action = "None" /> <!--  Negative signs must be spaced correctly -->
<Rule Id = "SA1022" Action = "None" /> <!--  Positive signs must be spaced correctly -->
<Rule Id = "SA1023" Action = "None" /> <!--  Dereference and access of symbols must be spaced correctly -->
<Rule Id = "SA1024" Action = "None" /> <!--  Colons must be spaced correctly -->
<Rule Id = "SA1025" Action = "None" /> <!--  Code must not contain multiple whitespace in arow -->
<Rule Id = "SA1026" Action = "None" /> <!--  Code must not contain space after new keyword in implicitly typed array allocation -->
<Rule Id = "SA1027" Action = "None" /> <!--  Use tabs correctly -->
<Rule Id = "SA1028" Action = "None" /> <!--  Code must not contain trailing whitespace -->
<Rule Id = "SA1100" Action = "None" /> <!--  Do not prefix calls with base unless local implementation exists -->
<Rule Id = "SA1101" Action = "None" /> <!--  Prefix local calls with this -->
<Rule Id = "SA1106" Action = "None" /> <!--  Code must not contain empty statements -->
<Rule Id = "SA1107" Action = "None" /> <!--  Code must not contain multiple statements on one line -->
<Rule Id = "SA1108" Action = "None" /> <!--  Block statements must not contain embedded comments -->
<Rule Id = "SA1109" Action = "None" /> <!--  Block statements must not contain embedded regions -->
<Rule Id = "SA1110" Action = "None" /> <!--  Opening parenthesis must be on declaration line -->
<Rule Id = "SA1111" Action = "None" /> <!--  Closing parenthesis must be on line of last parameter -->
<Rule Id = "SA1112" Action = "None" /> <!--  Closing parenthesis must be on line of opening parenthesis -->
<Rule Id = "SA1113" Action = "None" /> <!--  Comma must be on same line as previous parameter -->
<Rule Id = "SA1114" Action = "None" /> <!--  Parameter list must follow declaration -->
<Rule Id = "SA1115" Action = "None" /> <!--  Parameter must follow comma -->
<Rule Id = "SA1116" Action = "None" /> <!--  Split parameters must start on line after declaration -->
<Rule Id = "SA1117" Action = "None" /> <!--  Parameters must be on same line or separate lines -->
<Rule Id = "SA1118" Action = "None" /> <!--  Parameter must not span multiple lines -->
<Rule Id = "SA1119" Action = "None" /> <!--  Statement must not use unnecessary parenthesis -->
<Rule Id = "SA1120" Action = "None" /> <!--  Comments must contain text -->
<Rule Id = "SA1121" Action = "None" /> <!--  Use built in type alias -->
<Rule Id = "SA1122" Action = "None" /> <!--  Use string empty for empty strings -->
<Rule Id = "SA1123" Action = "None" /> <!--  Do not place regions within elements -->
<Rule Id = "SA1124" Action = "None" /> <!--  Do not use regions -->
<Rule Id = "SA1125" Action = "None" /> <!--  Use shorthand for nullable types -->
<Rule Id = "SA1126" Action = "None" /> <!--  Prefix calls correctly -->
<Rule Id = "SA1127" Action = "None" /> <!--  Generic type constraints must be on own line -->
<Rule Id = "SA1128" Action = "None" /> <!--  Constructor initializer must be on own line -->
<Rule Id = "SA1129" Action = "None" /> <!--  Do not use default value type constructor -->
<Rule Id = "SA1130" Action = "None" /> <!--  Use lambda syntax -->
<Rule Id = "SA1131" Action = "None" /> <!--  Use readable conditions -->
<Rule Id = "SA1132" Action = "None" /> <!--  Do not combine fields -->
<Rule Id = "SA1133" Action = "None" /> <!--  Do not combine attributes -->
<Rule Id = "SA1134" Action = "None" /> <!--  Attributes must not share line -->
<Rule Id = "SA1135" Action = "None" /> <!--  Using directives must be qualified -->
<Rule Id = "SA1136" Action = "None" /> <!--  Enum values should be on separate lines -->
<Rule Id = "SA1137" Action = "None" /> <!--  Elements should have the same indentation -->
<Rule Id = "SA1139" Action = "None" /> <!--  Use literal suffix notation instead of casting -->
<Rule Id = "SA1200" Action = "None" /> <!--  Using directives must be placed correctly -->
<Rule Id = "SA1201" Action = "None" /> <!--  Elements must appear in the correct order -->
<Rule Id = "SA1202" Action = "None" /> <!--  Elements must be ordered by access -->
<Rule Id = "SA1203" Action = "None" /> <!--  Constants must appear before fields -->
<Rule Id = "SA1204" Action = "None" /> <!--  Static elements must appear before instance elements -->
<Rule Id = "SA1205" Action = "None" /> <!--  Partial elements must declare access -->
<Rule Id = "SA1206" Action = "None" /> <!--  Declaration keywords must follow order -->
<Rule Id = "SA1207" Action = "None" /> <!--  Protected must come before internal -->
<Rule Id = "SA1208" Action = "None" /> <!--  System using directives must be placed before other using directives -->
<Rule Id = "SA1209" Action = "None" /> <!--  Using alias directives must be placed after other using directives -->
<Rule Id = "SA1210" Action = "None" /> <!--  Using directives must be ordered alphabetically by namespace -->
<Rule Id = "SA1211" Action = "None" /> <!--  Using alias directives must be ordered alphabetically by alias name -->
<Rule Id = "SA1212" Action = "None" /> <!--  Property accessors must follow order -->
<Rule Id = "SA1213" Action = "None" /> <!--  Event accessors must follow order -->
<Rule Id = "SA1214" Action = "None" /> <!--  Readonly elements must appear before non readonly elements -->
<Rule Id = "SA1216" Action = "None" /> <!--  Using static directives must be placed at the correct location -->
<Rule Id = "SA1217" Action = "None" /> <!--  Using static directives must be ordered alphabetically -->
<Rule Id = "SA1300" Action = "None" /> <!--  Element must begin with upper case letter -->
<Rule Id = "SA1301" Action = "None" /> <!--  Element must begin with lower case letter -->
<Rule Id = "SA1302" Action = "None" /> <!--  Interface names must begin with i -->
<Rule Id = "SA1303" Action = "None" /> <!--  Const field names must begin with upper case letter -->
<Rule Id = "SA1304" Action = "None" /> <!--  Non private readonly fields must begin with upper case letter -->
<Rule Id = "SA1305" Action = "None" /> <!--  Field names must not use hungarian notation -->
<Rule Id = "SA1306" Action = "None" /> <!--  Field names must begin with lower case letter -->
<Rule Id = "SA1307" Action = "None" /> <!--  Accessible fields must begin with upper case letter -->
<Rule Id = "SA1308" Action = "None" /> <!--  Variable names must not be prefixed -->
<Rule Id = "SA1309" Action = "None" /> <!--  Field names must not begin with underscore -->
<Rule Id = "SA1310" Action = "None" /> <!--  Field names must not contain underscore -->
<Rule Id = "SA1311" Action = "None" /> <!--  Static readonly fields must begin with upper case letter -->
<Rule Id = "SA1312" Action = "None" /> <!--  Variable names must begin with lower case letter -->
<Rule Id = "SA1313" Action = "None" /> <!--  Parameter names must begin with lower case letter -->
<Rule Id = "SA1314" Action = "None" /> <!--  Type parameter names must begin with t -->
<Rule Id = "SA1400" Action = "None" /> <!--  Access modifier must be declared -->
<Rule Id = "SA1401" Action = "None" /> <!--  Fields must be private -->
<Rule Id = "SA1402" Action = "None" /> <!--  File may only contain asingle type -->
<Rule Id = "SA1403" Action = "None" /> <!--  File may only contain asingle namespace -->
<Rule Id = "SA1404" Action = "None" /> <!--  Code analysis suppression must have justification -->
<Rule Id = "SA1405" Action = "None" /> <!--  Debug assert must provide message text -->
<Rule Id = "SA1406" Action = "None" /> <!--  Debug fail must provide message text -->
<Rule Id = "SA1407" Action = "None" /> <!--  Arithmetic expressions must declare precedence -->
<Rule Id = "SA1408" Action = "None" /> <!--  Conditional expressions must declare precedence -->
<Rule Id = "SA1409" Action = "None" /> <!--  Remove unnecessary code -->
<Rule Id = "SA1410" Action = "None" /> <!--  Remove delegate parenthesis when possible -->
<Rule Id = "SA1411" Action = "None" /> <!--  Attribute constructor must not use unnecessary parenthesis -->
<Rule Id = "SA1412" Action = "None" /> <!--  Store files as utf8 -->
<Rule Id = "SA1413" Action = "None" /> <!--  Use trailing commas in multi line initializers -->
<Rule Id = "SA1500" Action = "None" /> <!--  Braces for multi line statements must not share line -->
<Rule Id = "SA1501" Action = "None" /> <!--  Statement must not be on asingle line -->
<Rule Id = "SA1502" Action = "None" /> <!--  Element must not be on asingle line -->
<Rule Id = "SA1503" Action = "None" /> <!--  Braces must not be omitted -->
<Rule Id = "SA1504" Action = "None" /> <!--  All accessors must be single line or multi line -->
<Rule Id = "SA1505" Action = "None" /> <!--  Opening braces must not be followed by blank line -->
<Rule Id = "SA1506" Action = "None" /> <!--  Element documentation headers must not be followed by blank line -->
<Rule Id = "SA1507" Action = "None" /> <!--  Code must not contain multiple blank lines in arow -->
<Rule Id = "SA1508" Action = "None" /> <!--  Closing braces must not be preceded by blank line -->
<Rule Id = "SA1509" Action = "None" /> <!--  Opening braces must not be preceded by blank line -->
<Rule Id = "SA1510" Action = "None" /> <!--  Chained statement blocks must not be preceded by blank line -->
<Rule Id = "SA1511" Action = "None" /> <!--  While do footer must not be preceded by blank line -->
<Rule Id = "SA1512" Action = "None" /> <!--  Single line comments must not be followed by blank line -->
<Rule Id = "SA1513" Action = "None" /> <!--  Closing brace must be followed by blank line -->
<Rule Id = "SA1514" Action = "None" /> <!--  Element documentation header must be preceded by blank line -->
<Rule Id = "SA1515" Action = "None" /> <!--  Single line comment must be preceded by blank line -->
<Rule Id = "SA1516" Action = "None" /> <!--  Elements must be separated by blank line -->
<Rule Id = "SA1517" Action = "None" /> <!--  Code must not contain blank lines at start of file -->
<Rule Id = "SA1518" Action = "None" /> <!--  Use line endings correctly at end of file -->
<Rule Id = "SA1519" Action = "None" /> <!--  Braces must not be omitted from multi line child statement -->
<Rule Id = "SA1520" Action = "None" /> <!--  Use braces consistently -->
<Rule Id = "SA1600" Action = "None" /> <!--  Elements must be documented -->
<Rule Id = "SA1601" Action = "None" /> <!--  Partial elements must be documented -->
<Rule Id = "SA1602" Action = "None" /> <!--  Enumeration items must be documented -->
<Rule Id = "SA1603" Action = "None" /> <!--  Documentation must contain valid xml -->
<Rule Id = "SA1604" Action = "None" /> <!--  Element documentation must have summary -->
<Rule Id = "SA1605" Action = "None" /> <!--  Partial element documentation must have summary -->
<Rule Id = "SA1606" Action = "None" /> <!--  Element documentation must have summary text -->
<Rule Id = "SA1607" Action = "None" /> <!--  Partial element documentation must have summary text -->
<Rule Id = "SA1608" Action = "None" /> <!--  Element documentation must not have default summary -->
<Rule Id = "SA1609" Action = "None" /> <!--  Property documentation must have value -->
<Rule Id = "SA1610" Action = "None" /> <!--  Property documentation must have value text -->
<Rule Id = "SA1611" Action = "None" /> <!--  Element parameters must be documented -->
<Rule Id = "SA1612" Action = "None" /> <!--  Element parameter documentation must match element parameters -->
<Rule Id = "SA1613" Action = "None" /> <!--  Element parameter documentation must declare parameter name -->
<Rule Id = "SA1614" Action = "None" /> <!--  Element parameter documentation must have text -->
<Rule Id = "SA1615" Action = "None" /> <!--  Element return value must be documented -->
<Rule Id = "SA1616" Action = "None" /> <!--  Element return value documentation must have text -->
<Rule Id = "SA1617" Action = "None" /> <!--  Void return value must not be documented -->
<Rule Id = "SA1618" Action = "None" /> <!--  Generic type parameters must be documented -->
<Rule Id = "SA1619" Action = "None" /> <!--  Generic type parameters must be documented partial class -->
<Rule Id = "SA1625" Action = "None" /> <!--  Element documentation must not be copied and pasted -->
<Rule Id = "SA1626" Action = "None" /> <!--  Single line comments must not use documentation style slashes -->
<Rule Id = "SA1627" Action = "None" /> <!--  Documentation text must not be empty -->
<Rule Id = "SA1628" Action = "None" /> <!--  Documentation text must begin with acapital letter -->
<Rule Id = "SA1629" Action = "None" /> <!--  Documentation text must end with aperiod -->
<Rule Id = "SA1630" Action = "None" /> <!--  Documentation text must contain whitespace -->
<Rule Id = "SA1631" Action = "None" /> <!--  Documentation must meet character percentage -->
<Rule Id = "SA1632" Action = "None" /> <!--  Documentation text must meet minimum character length -->
<Rule Id = "SA1642" Action = "None" /> <!--  Constructor summary documentation must begin with standard text -->
<Rule Id = "SA1643" Action = "None" /> <!--  Destructor summary documentation must begin with standard text -->
<Rule Id = "SA1644" Action = "None" /> <!--  Documentation headers must not contain blank lines -->
<Rule Id = "SA1645" Action = "None" /> <!--  Included documentation file does not exist -->
<Rule Id = "SA1646" Action = "None" /> <!--  Included documentation xpath does not exist -->
<Rule Id = "SA1647" Action = "None" /> <!--  Include node does not contain valid file and path -->
<Rule Id = "SA1648" Action = "None" /> <!--  Inherit doc must be used with inheriting class -->
<Rule Id = "SA1649" Action = "None" /> <!--  File name must match type name -->
<Rule Id = "SA1650" Action = "None" /> <!--  Element documentation must be spelled correctly -->
<Rule Id = "SA1651" Action = "None" /> <!--  Do not use placeholder elements -->
<Rule Id = "SX1101" Action = "None" /> <!--  Do not prefix local members with this -->
<Rule Id = "SX1309" Action = "None" /> <!--  Field names must begin with underscore -->
<Rule Id = "SX1309S" Action = "None" /> <!--  Static field names must begin with underscore -->

@blairconrad
Copy link
Contributor Author

blairconrad commented May 24, 2018

Hmmm. The SX rules are disabled by default. Lemme see what we can do about that PowerShell.

Get-ChildItem -Recurse -Filter *.cs |                                                      
    ForEach-Object { $_.Name } |                                                           
    Where-Object { $_ -match  "^SA[0-9]{4}" } |                                            
    Sort-Object |                                                                          
    ForEach-Object {                                                                       
        $ruleId = $_.Substring(0, 6)                                                       
        $description = $_.Substring(6, $_.Length-9) -creplace "([a-z])([A-Z])",'$1 $2'     
        $description = ($description[0] + $description.Substring(1).ToLower())             
        Write-Output "<Rule Id = `"$ruleId`" Action = `"None`" /> <!-- $description -->"   
    }                                                                                                                                                          

Could probably be improved, but I tend to tinker with these things just until they work and then drop it.

The main aim is to improve IDE performance when
writing the code.
@zvirja zvirja changed the title [WIP] Use Stylecop Enable Stylecop analysis May 26, 2018
@zvirja
Copy link
Member

zvirja commented May 26, 2018

@blairconrad I've used your code snippet and disabled code analysis for the regular development. Thanks a lot 👍 (You know, you do a lot for the project, so it's harder and harder for me to find the "thanks" words for you 😖)

@moodmosaic Please take a look if you have time. Basically, the following changes were applied:

  • Enabled code style verification except a few rules. We fail for verification build if any code style rule is violated (triggered when the full build is requested via build scripts or CI).
  • Suppressed some more rules for the tests project, as it's hard to follow them there.
  • Tuned R# configuration to comply with the StyleCop rules.
  • Disabled code analysis for non-verification builds. It allows IDEs to work faster and compile code quicker.

No extra tooling is required to work with analysis - we use Roslyn analyzers imported via NuGet package.
I reviewed all the rules and must admin that most of them are adequate, so I'm fine with them 😉

If you don't have free time and trust my tastes - let me know and I'm fine to proceed with this further 😅

@blairconrad
Copy link
Contributor Author

@blairconrad I've used your code snippet and disabled code analysis for the regular development. Thanks a lot 👍 (You know, you do a lot for the project, so it's harder and harder for me to find the "thanks" words for you 😖)

@zvirja, no problem, and I help because I enjoy it. That and a simple "thanks" are more than enough!

Disabled code analysis for non-verification builds. It allows IDEs to work faster and compile code quicker.

This is true, but increases the chance that you'll get PRs that contain violations (if contributors don't run the command-line build).
Another consideration is that by disabling the rules when contributors build in the IDE, you're robbing them of the code fixes that the analyzer provides.
Just now, as an experiment, I added an extra space after a new, and built in the IDE, and it was happy, but then build.cmd broke, leaving me to interpret the error message and hunt down the "multiple whitespace characters in a row.". Had the analyzer been on, I'd've seen the squiggles in the IDE maybe even before compiling.
Me, I enjoy the convenience of the code fixes, and would pay the "compilation slowdown tax", but that's just my preference. I'm not trying to sway you (hmm... actually I think I am, but you know, just in a hopefully helpful "make sure you've considered all the corners" kind of way, not in a "You should do this! I have rights!" kind of way). Thanks for listening!

@moodmosaic
Copy link
Member

@zvirja, I'll go through this starting from now and during the rest of the day (on and off).

/// receives a parameter of the same type, so using it as a factory inevitably
/// This type contains a method that returns an instance of the same type
/// so that it looks like a Factory Method. In fact, it is not because it
/// receives a parameter of the same type, so using it as a factory inevitably
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I have Sublime Text do this for me automatically on save, but happy to see this being applied here.

{
return new TypeWithCastOperatorsWithoutPublicConstructor();
}

public static explicit operator TypeWithCastOperatorsWithoutPublicConstructor(string _)
public static explicit operator TypeWithCastOperatorsWithoutPublicConstructor(string ignored)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ignored is more clear than _ so it's no confused with the wildcard pattern.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -2,6 +2,6 @@
{
public enum EmptyEnum
{
//this must not contain any values
// this must not contain any values
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, it'd be nice if we could enforce that comments start with upper case and end with period:

// This must not contain any values.

But I'll leave this up to you guys to devicedecide 😃

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No such rule so far is present in the library. If it appears - we'll definitely enable it 😉

Copy link
Member

@moodmosaic moodmosaic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed from the end up to the middle and I pretty much find this awesome.

I trust that @zvirja and @blairconrad did a great job already, so I'm not going to review further. (After all, it's a massive PR.)

The overall direction and idea is clear anyway.


For a smaller codebase I wouldn't agree on having something like this, but for a fairly sized codebase (like AutoFixture) I think there's some value on being consistent.

I know at least one person that's OCDish like me, and that's @ecampidoglio so I thought I should /cc him 😃

@ecampidoglio
Copy link
Member

As @moodmosaic correctly pointed out, I'm all in for consistency. 😉

Well done @blairconrad and thank you for your contribution – this one gets a big thumbs up from me. 👍

@zvirja
Copy link
Member

zvirja commented Jun 4, 2018

Me, I enjoy the convenience of the code fixes, and would pay the "compilation slowdown tax", but that's just my preference. I'm not trying to sway you (hmm... actually I think I am, but you know, just in a hopefully helpful "make sure you've considered all the corners" kind of way, not in a "You should do this! I have rights!" kind of way). Thanks for listening!

@blairconrad I had hard times thinking about this and making the decision 😫 I do understand all the concerns and any decision is a trade-off.

I tried also to play with things and found that it incredibly slows down the IDE. It isn't comfortable anymore to write the code when analyzers are enabled. Also the compilation time increased 😟 While the quick fixes are convenient, it doesn't worth it as for me. Additionally I hope that most of the contributors have R# enabled and its automatic formatting will fix most the typos.

Therefore, I'd like to proceed with the code analysis disabled by default. There is always an option to switch to the Verify configuration and get the quick fixes there if required. For me it looks like a good trade-off. The only drawback here is that solution should be re-loaded in VS to detect the changed active rule set, but it's a VS specifics that might improve in future.

I've added a hint to the error message when verification build fails, so users knows about the aforementioned option.

@blairconrad
Copy link
Contributor Author

And here I was blaming ReSharper for my sluggish IDE… 😉
Thanks for the response. Good luck!

@zvirja
Copy link
Member

zvirja commented Jun 4, 2018

And here I was blaming ReSharper for my sluggish IDE… 😉

Yep. Usually I use Rider and it performs relatively well. Tried to use VS for a couple of hours and it's impossible to work there. 20+ sec of freeze after each All.sln build (likely, because R# tries to re-discover the unit tests). A few more major VS releases and it will be literally impossible to use it together with R# 😟

@zvirja zvirja merged commit b9a3f4d into AutoFixture:master Jun 4, 2018
@zvirja
Copy link
Member

zvirja commented Jun 4, 2018

@blairconrad Finally merged! One more time thanks for the help on this - I feel how the brighter future is now one step closer 😊

@blairconrad blairconrad deleted the stylecop branch June 4, 2018 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants