Declarations (method/class/property/namespace/etc) must not contain embedded comments

[macpak]

I would like to propose a new diagnostic:

Category: ReadabilityRules Rule: Declarations must not contain embedded comments Description: When declaring a class/method/namespace/property, we shouldn't allow to put a comment between the declaration and the opening curly bracket

Rationale: In #42 we catch situations where a commment is put between a statement and the opening curly bracket. I think the same rule should apply to declarations. For example, code above should not be allowed:

public class IndexTests
//aa
{
    public int Prop
    //aa
    {
        get
        //bb
        {
            return 0;
        }

        set
        //cc
        {

        }
    }
  }

[sharwell]

I am in favor of this, but I'm not sure how we're planning on assigning IDs to new diagnostics. For my proposal, I implemented it using the next available number in that category.

What should our policy/procedure be regarding additions?

[macpak]

I'd follow your pattern and choose the next number available. For this one, it'd be SA1127.

[vweijsters]

Choosing the next number sounds fine. Maybe we should mark these as SX category as well, since they are not original StyleCop?

[sharwell]

@vweijsters Originally I was thinking of SX rules as conflicting in some way with the SA rules. It's easy to see that this rule wouldn't conflict with any. It's risky using an SAxxxx identifier, but it seems no new rules have been defined in at least 3 years now, so perhaps we are safe?

Another option is to initially implement it as an SX rule (or some one other designation indicating a preliminary or trial rule), and then promote it to an SA rule after it's been vetted/used in practice by a wider audience.

[vweijsters]

@sharwell I like the option of using SX for new rules, where the X means experimental stuff, including some form of promoting mechanism.

The alternatives proposal (#600) could be also be done by appending a letter code to the rule, for example: SA1100 for the original one, SA1100a for the first alternative, SA1100b for a second alternative, etc.

[Przemyslaw-W]

I am not an contributor here, I am big StyleCop fan and I used to contribute lots of issues to original SC. So here are my thoughts:

I like SX for "alternative" rules. The opinionated nature of original SC was its biggest weakness IMHO. But I would not bother with new rules being introduced as SX and then promoted to SA. First - original SC is effectively dead. I don't know what happened to andyr and I wish him all the best. Unfortunately, it looks like bus factor hit original SC project. As a result - there will be no conflicts with your work and original SC. Second - changing identifier later will be breaking change - e.g. suppressions with #pragma warning disable SX1234 will stop working if you will change SX1234 to SA1234. Third

• what will you do with new rule which comes with alternative flavour as well? For these reasons, for new rules, I would just go with SA followed by next available number in category. Of course, new rules should be as much grouped as possible - e.g. possible new rules for ternary operator should be added together and get adjacent numbers.

Keep up the good work guys.

[sharwell]

@Przemyslaw-W Thanks for your feedback.

I am not an contributor here

All it takes to be a contributor here is providing feedback. :+1:

For these reasons, for new rules, I would just go with SA followed by next available number in category.

Everything you wrote makes perfect sense. Since the primary reason for a "trial" identifier and "promotion" would be making sure more people give feedback before finalizing a rule, how would you feel about keeping a trial period, but adding a statement that a rule will be promoted to its final form before it gets included in a stable release (e.g. 1.0.0 is stable but 1.0 Alpha 5 is a preview release). More specifically:

[Proposed Rule:] The identifier for a diagnostic cannot change after the diagnostic is enabled by default in a stable release.

Even if we assign SA rules from the start for new diagnostics, I think the above rule is a good idea to document (and of course follow).

[Przemyslaw-W]

I think trial identifiers have merit when new rule comes with two flavours. So the community can help to decide which one goes with SA and which with SX. If there is only one flavour then the only decision to make is whether to enable the rule by default. And identifier is not important in such case

• so go with SA by default.

I also would not bother with SA1234a, SA1234b, SA1234c. SC rules are boolean by its nature. More complex styles can be build by enabling/disabling proper set of yes/no rules. So SA and SX are enough IMHO.

And another thing - you guys start from scratch so you can "deviate" from the original SC rules. E.g. there was common complaint for SA1513 - it was reported for closing } in get and add accessor. Do you consider splitting original rules into few? here I would see SA1513 splitted into three:

• closing } for type or member declaration (class, struct, interface, method, property, event)
• closing } for accessor (get, set, add, remove)
• closing } for code block (if, else, while, for, do, try, catch, finally etc)

Similar approach for rule which triggered this thread:

• comments inside type or member declaration
• comments inside accessor declaration
• comments inside block declaration

Basically, I think more granurality is needed. I don't thing separate rule for method, property, etc. is necessary. But above 3 groups would be more than welcome. Maybe here the "a", 'b", "c" flavours could be applied:

• SA1513a - need blank line after closing } in type or member declaration (or SA1513m - m like member)
• SX1513a - no blank line after closing } in member declaration (or SX1513m - m like member)
• SA1513b - need blank line after closing } in accessor declaration (or SA1513a - a like accessor)
• SX1513b - no blank line after closing } in accessor declaration (or SX1513a - a like accessor)
• SA1513c - need blank line after closing } in code block (or SA1513b - b like block)
• SX1513c - no blank line after closing } in code block (or SX1513b - b like block)

If such scheme would be introduced, then SA1513a rule would be suppressed in code by both - "#pragma warning disable SA1513" and "#pragma warning disable SA1513a"

What do you think?

2015-04-03 23:53 GMT+02:00 Sam Harwell notifications@github.com:

@Przemyslaw-W https://github.com/Przemyslaw-W Thanks for your feedback.

I am not an contributor here

All it takes to be a contributor here is providing feedback. [image: :+1:]

For these reasons, for new rules, I would just go with SA followed by next available number in category.

Everything you wrote makes perfect sense. Since the primary reason for a "trial" identifier and "promotion" would be making sure more people give feedback before finalizing a rule, how would you feel about keeping a trial period, but adding a statement that a rule will be promoted to its final form before it gets included in a stable release (e.g. 1.0.0 is stable but 1.0 Alpha 5 is a preview release). More specifically: [Proposed Rule:] The identifier for a diagnostic cannot change after the diagnostic is enabled by default in a stable release.

Even if we assign SA rules from the start for new diagnostics, I think the above rule is a good idea to document (and of course follow).

— Reply to this email directly or view it on GitHub https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/605#issuecomment-89431572 .

[AArnott]

I like the proposal for the new comments rule