Enforce more code style in Compilers area

[Youssef1313]

No description provided.

[CyrusNajmabadi]

Please do not make code style changes to the compiler. If you want, you can open an issue where the topic can be discussed and the compiler team can make a decision here

[mavasani]

Please do not make code style changes to the compiler. If you want, you can open an issue where the topic can be discussed and the compiler team can make a decision here

@CyrusNajmabadi I believe this is something that the compiler has already shown interest in - uniform code style enforcement in the Roslyn repo.

[mavasani]

@Youssef1313 Can you fix each ID using a FixAll application in a single commit instead of separate individual fixes?

[jaredpar]

I believe this is something that the compiler has already shown interest in - uniform code style enforcement in the Roslyn repo.

Yes we have shown this interest and there are a number of styles we're going to be making uniform.

At the same time though we don't generally accept PRs that are style only. It's actively called out in our Contributing document.

DO NOT submit large code formatting changes without discussing with the team first.

Was this discussed somewhere I missed?

[Youssef1313]

@Youssef1313 Can you fix each ID using a FixAll application in a single commit instead of separate individual fixes?

Sure. I've selected a FixAll for solution in Visual Studio, but seems to be taking too long.

[Youssef1313]

Yes we have shown this interest and there are a number of styles we're going to be making uniform.

At the same time though we don't generally accept PRs that are style only. It's actively called out in our Contributing document.

@jaredpar Hmm, So I should close the PR? or enforcing styles with .editorconfig will be an exception?

[mavasani]

Good point @Youssef1313. I'll let @CyrusNajmabadi @333fred chime in, I am personally fine with any option here.

[333fred]

Alright. The pretty printer (the thing that formats VB code on line commit) is the source of truth here. It says Partial first, so Partial is first. Let's revert the editorconfig change.

[333fred]

Mostly looking good. The BoundNodeWriter will have to be updated for Correctness to pass: https://github.com/dotnet/roslyn/blob/master/src/Tools/Source/CompilerGeneratorTools/Source/BoundTreeGenerator/BoundNodeClassWriter.cs#L311

[Youssef1313]

@333fred I updated it. This is used for auto-generated code right?

[333fred]

@333fred I updated it. This is used for auto-generated code right?

Yes, it generates the bound nodes based on the BoundNodes.xml files for each language.

[333fred]

LGTM (commit 37). @dotnet/roslyn-compiler for a second review.

[mavasani]

@dotnet/roslyn-compiler for a second review.

[jcouv]

LGTM Thanks (iteration 38)

[333fred]

I'm going to run CI again to make sure that there were no conflicts introduced between the last update and now.

[333fred]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[ghost]

Hello @333fred!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[ghost]

Auto-approval

[jcouv]

Argh, the bot doesn't know to squash... Let's try to squash next time. Thanks

[333fred]

Thanks Youssef!