Skip to content
This repository was archived by the owner on Jan 12, 2024. It is now read-only.

Conversation

@bamarsha
Copy link
Contributor

@bamarsha bamarsha commented Jun 30, 2020

This is a first step to resolving #342. I added the StyleCop.Analyzers package to the CommandLineTool project to try out StyleCop's formatting rules. I plan to add StyleCop to the rest of the projects in qsharp-compiler, but I wanted to make a smaller PR with just this project to get feedback on the style choices I made.

StyleCop's default rules are extensive, and highly opinionated, so I ended up disabling several rules that seemed too strict for us. Those disabled rules are in src/Common/CodeAnalysis.ruleset.

I also added an .editorconfig file to override Visual Studio's default behavior and to enforce a style rule that StyleCop doesn't have:

dotnet_style_qualification_for_{event,field,method,property} = true
By default, Visual Studio suggests replacing this.Foo with Foo. @bettinaheim and I decided that we want to require this. in all cases. This is already the default for StyleCop, but Visual Studio's behavior needs to be set separately.

dotnet_diagnostic.IDE0002.severity = warning
This promotes Visual Studio's code action for simplifying qualified names to a warning. I added this to simplify references to static members within the same class. So if class Foo has a static method Bar, for example, I think we should call that method from within Foo by writing Bar() instead of Foo.Bar(). I think we should use this style because:

  • Normally, with unqualified references, it's ambiguous whether it's an instance member or a static member. Since we require instance members to be prefixed with this., we don't need to prefix static members - we know any unprefixed reference is static.
  • Static members are analogous to members of a module in F# - we don't prefix references to other things in the same module in F#, so I don't think we should do that in C#, either.
  • Prefixes are necessary when the static member is in a different class - adding an unnecessary prefix can create the impression that the reference is to a different class, which can be confusing.

Copy link
Contributor

@bettinaheim bettinaheim left a comment

Choose a reason for hiding this comment

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

All in all looks good to me! Thanks for looking into this!

@cgranade
Copy link
Contributor

If you're adding an .editorconfig file, may I suggest adding a .vscode/extensions.json file that suggests the EditorConfig extension for VS Code as well? That won't have any effect other than to notify a contributor that the repo supports EditorConfig, but even that may be a helpful signal to provide.

Copy link
Contributor

@ScottCarda-MS ScottCarda-MS left a comment

Choose a reason for hiding this comment

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

I'm very excited for these changes! I've been wanting something like this for a while. Good job!

@bamarsha
Copy link
Contributor Author

bamarsha commented Jul 1, 2020

@filipw Do you know what the best way is to show Roslyn analyzers in OmniSharp (for VS Code support)? It was not showing them by default, so I added this omnisharp.json file:

{
    "RoslynExtensionsOptions": {
        "enableAnalyzersSupport": true
    }
}

but now it shows all StyleCop warnings, even the ones that were disabled by .editorconfig.

@filipw
Copy link
Contributor

filipw commented Jul 1, 2020

editorconfig has to be opted into separately in OmniSharp, add this to omnisharp.json too, next to the section you already have to enable analyzers:

    "FormattingOptions": {
        "enableEditorConfigSupport": true
    }

@bamarsha
Copy link
Contributor Author

bamarsha commented Jul 1, 2020

Thanks!

@bamarsha bamarsha merged commit 3c9bde9 into master Jul 9, 2020
@bamarsha bamarsha deleted the samarsha/stylecop branch July 9, 2020 03:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants