Stylecopanalyzers: Question on validity of SA1413

[jcasale]

Hi,
Just curious how the StyleCopAnalyzers specific versus the original StyleCop rules are debated and assessed? Its possible I am missing the obvious but I wonder how SA1413 actually improves maintainability? I feel it makes code confusing leaving a developer to wonder if someone possibly removed a line in error? With the trailing comma removed it requires a more deliberate change.

[pdelvo]

All of these rules are implemented because of subjective reason. The reasons for implementing this rule can be seen in this issue: https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/1121.

[jcasale]

I did find that thread but it does not explain how the rule assists in maintainability. I can discount the weight of my opinion, I am only a grateful user who doesn't contribute but this rule the first which I can honestly say I didn't see merit in but actually thought it promoted the opposite. For even the stylistic rules that are subjective, I have always been able to justify them on the premise that consistency and uniformity is important.

The real purpose of my inquiry was simply to learn how the rules are voted in and discussed. My original post describes how I see this rule to be counter productive and promoting of confusion.

[sharwell]

@jcasale Note that this is a maintainability rule as opposed to a readability rule. The advantages of including the comma have to do with the work required to make a change to the item falling at the end of a list (removing it, adding to it, or reordering it). With SA1413 in place, these changes are slightly less work for the developer on average. They are also less work for a reviewer because you aren't including lines in your diff just for adding a comma where one wasn't before.

I voted in favor of the rule for the following combination of reasons:

1. I've hit both of the two cases originally described with the proposal multiple times
2. When I encountered the same situation but a trailing comma was in place, it was less work to make whatever change I was doing

We don't have a formal process defined/documented for new rules. StyleCop is opinionated on a subject matter that the compiler really doesn't care about, so typically we try to weigh the following to determine what/if opinion we'll have:

1. The likelihood of a situation applying to a user's code
2. Whether or not StyleCop Analyzers already has a rule covering the situation
3. Whether or not StyleCop Analyzers already has a rule covering a similar situation, such that the opinions on somewhat related rules can be consistent

[jcasale]

@sharwell Thanks for the detailed reply.

[ghost]

Sam recommended that I turn off this rule if I don't like it. I think your rational is backwards. At this stage in the product's maturity, you should be telling the people who want this new rule to turn it on. Jamming something this trivial this down our throats when we've invested weeks if not months of man-hours to get compliant just adds aggravation to my day and I don't see the advantage.

[sharwell]

@DRAirey1 We do have a high bar for several aspects of new rules. In particular:

1. This rule only affects an issue in source code which previously had no rule (i.e. there are no conflicts with existing rules)
2. The analysis is exceptionally fast
3. The code fix is exceptionally fast and reliable ("trustworthy")

:bulb: In general we prefer to not add a new rule unless these items can be met and the rule enabled by default.

The time required to update a codebase for compliance with the new rule is minimal, but the new rule may also be disabled either due to personal preference or for any other reason.

[berkeleybross]

Just to weigh in my opinion, I would prefer to see this rule disabled by default, and the opposite rule suggested in #1121 should be implemented and enabled by default. Primarily this is to be consistent with tools like ReSharper which flags trailing commas as redundant and suggests they be removed.

Additionally, I think this rule only improves edit-ability slightly (and tools like ReSharper mitigate most of the complaints), whilst readability is impacted (albeit only slightly as well). I think its generally accepted that code is read many more times than its written, so we shouldn't promote write-ability at the sake of readability.

I've been following this project for some time, but not dipped my toes into actually helping yet. I would be happy to start here if there are no objections, although I may need a few pointers to get started :)

[sharwell]

@berkeleybross At this point I would not expect the default behavior of this rule to change. A specific proposal to add an option (with values require/allow/omit like seen in #2050) might be entertained as long as the default is require. If you are interested in this, the best way to start would be with an issue proposing the new option.

[JimBobSquarePants]

Chipping in here as I was very surprised to discover this rule existed. A trailing comma in a list is a syntactic abberation that I would expect most developers (and English language speakers) to object to and actively remove. I implore you to reconsider it.

[ghost]

"but the new rule may also be disabled either due to personal preference or for any other reason."
I don't know if others see this as black-and-white as I do, but the minute you disable any of the rules for 'personal' reasons, you open up a can-of-worms. Why should the lead developer get to chose what rules he likes and doesn't like? Let's have a committee where we'll discuss which rules we're going to adopt as our standard. Wait, the build engineering group also has a list of preferred rules, let's have a meeting to discuss which ones we're both going to use.

FWIW, If I allow my developers to select personal preferences, there's not a whole lot of benefit to using StyleCop if I've got several 'standards' floating around the shop.

[stealthcold]

We really hated this rule.