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

Consider using Coding Style and .editorconfig #342

Closed
rstm-sf opened this issue Feb 26, 2020 · 7 comments · Fixed by #769
Closed

Consider using Coding Style and .editorconfig #342

rstm-sf opened this issue Feb 26, 2020 · 7 comments · Fixed by #769
Assignees
Labels
area: SDK Quantum SDK used to build Q# projects enhancement New request or suggestion for an improvement good first issue Good for newcomers

Comments

@rstm-sf
Copy link
Contributor

rstm-sf commented Feb 26, 2020

Hello!

Please, consider using Coding Style and .editorconfig. Because it’s unusual to see, for example, the following code

foreach (var diag in this.VerifiedCompilation?.Diagnostics() ?? Enumerable.Empty<Diagnostic>())
{ this.LogAndUpdate(ref this.CompilationStatus.Validation, diag); }

@rstm-sf rstm-sf added the enhancement New request or suggestion for an improvement label Feb 26, 2020
@bamarsha
Copy link
Contributor

Thanks for the suggestion! We've discussed standardizing the coding style recently. An .editorconfig file sounds like a good idea. It looks like a lot of editors have support for it, including Visual Studio and Visual Studio Code.

We're also interested in tools to automatically check code style, for example as part of the automated builds for PRs. It looks like dotnet-format can do this, but we're open to other suggestions.

@bamarsha bamarsha self-assigned this Feb 27, 2020
@bamarsha bamarsha added the area: SDK Quantum SDK used to build Q# projects label Feb 27, 2020
@filipw
Copy link
Contributor

filipw commented Feb 27, 2020

In C# extension in VS Code, editorconfig support is an opt in flag for OmniSharp (https://www.strathweb.com/2019/07/editorconfig-support-in-omnisharp-and-c-extension-vs-code/), we hide most of these newer features behind an opt-in flag on order to keep the base featureset is as lightweight as possible. but yes it definitely makes sense to me too!

For enforcing code style at build time you can also have a look at Microsoft.CodeAnalysis.CSharp.CodeStyle prerelease package (only exists on Roslyn CI feed). It ships support for whitespace (IDE0055) violations (same as those covered by dotnet-format) in a regular analyzer package and reads the settings from editorconfig file.

@cgranade
Copy link
Contributor

We actually have an editorconfig for the samples repo for precisely this reason. We've looked into using dotnet-format with precommit before to help with code style, but to limited success.

@bamarsha
Copy link
Contributor

I'm starting to work on an .editorconfig file in the marshallsa/editorconfig branch. Some mixed results so far.

First, it seems like the only way to reformat @rstm-sf's foreach example is to set csharp_preserve_single_line_blocks = false. However, this also reformats auto-properties that are defined on one line:

public int Foo { get; set; }

becomes

public int Foo
{
    get; set;
}

I think this is unnecessary, but I can't find a way to apply the reformatting to foreach without also applying it to auto-properties. So it's not ideal, but maybe worth it?

Second, it seems like Visual Studio does not support reformatting F# files at all. So unfortunately we might have to manually enforce an F# coding style. At least it could still be a good idea to write out some of the rules explicitly so we can refer back to them (probably based on the F# style guide with a few modifications), unless someone knows another way to auto-format F#.

@rstm-sf
Copy link
Contributor Author

rstm-sf commented Mar 28, 2020

It seems to me that the following option looks better csharp_preserve_single_line_blocks = false :)

It seems that I was mistaken in that I looked at the settings of editorconfig from far: for such a case it is difficult to choose an option. Maybe because it is not popular? Before that, I saw two equivalent options:

for ()
    smth

and

for ()
{
    smth
}

But this was the first time I met such an option and it seems to me that it is overloaded: trying to fit everything on one line only complicates further support

@rstm-sf
Copy link
Contributor Author

rstm-sf commented Mar 28, 2020

Second, it seems like Visual Studio does not support reformatting F# files at all. So unfortunately we might have to manually enforce an F# coding style. At least it could still be a good idea to write out some of the rules explicitly so we can refer back to them (probably based on the F# style guide with a few modifications), unless someone knows another way to auto-format F#.

There is a Fantomas project. It also has an issue supporting editorconfig fsprojects/fantomas#650

@bamarsha
Copy link
Contributor

We reformatted all of the C# code using StyleCop, so the code sample mentioned in the original post has been fixed. :)

The F# code still has no formatting rules currently, though, so I'll leave this issue open until we do. Thanks @rstm-sf for pointing out Fantomas!

@bamarsha bamarsha linked a pull request Dec 8, 2020 that will close this issue
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: SDK Quantum SDK used to build Q# projects enhancement New request or suggestion for an improvement good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants