Tagging @dotnet/roslyn-ide @sharwell @mavasani @heejaechang

Commit 516358c looks fine, but marking as Needs Design Review to discuss the indentation guide used in d7f4ea7959967203624aacc756208a99e20ee425.

📝 Alternately, the latter could be removed from this PR to unblock it.

I woudl prefer to not decouple. It pushes people away from always being willing to clean up anything they run into along the way as they do work. Please review at earliest convenience. Hopefully these sorts of changes in teh future do not cause an unnecessarily large impact on PR time.

Tagging @jinujoseph @kuhlenh

@sharwell @CyrusNajmabadi - I think both of you are right in your own way, and missing piece here is accurate guidelines here for both the PR reviewers and contributors.

1. Recommended code style guidelines to be followed. For example, here it would clarify whether our team prefers the current indentation style or the proposed indentation style.
2. Steps to take when there is a disagreement on some part of the PR change. These should be categorized based on whether the reviewer feels the comment is a blocking one for the PR or not, which would determine if the contentious change needs to be decoupled from the current PR OR retained in the current PR with a tracking issue for a follow-up discussion and PR (if needed) OR needs to resolved and blocks PR until then.

As a start, our team is currently working on a proposed set of guidelines for PR contributions to IDE analyzers/fixers/refactorings (which have most community interaction). We hope to discuss and agree upon these guidelines soon and then open it for public feedback. Once finalized, we will post it on GitHub and try these across few PRs, and iterate towards the optimal set.

For this specific concern my take is as follows:

1. There is no current guideline that determines if the current indentation style or the proposed indentation style is the one we want to follow, so we need a tracking design issue. @sharwell can you please file one?
2. Until we have documented what indentation style we want, we should not pollute any PR with feedback comments proposing or opposing either.
3. Additionally, I sees no reason to block the PR OR request any changes on the PR on this specific issue OR any issue where our team has not agreed upon the guidelines we want to follow. We should instead fast track:
   1. Discussion on design issue to resolve it
   2. Document it so future PRs follow them from start and
   3. Try to have cleanup PRs that change our code base to meet the recommended style as lot of contributors look at existing code rather then documentation.

@mavasani I like a lot of that. I will say this though:

Discussion on design issue to resolve it
Document it so future PRs follow them from start and
Try to have cleanup PRs that change our code base to meet the recommended style as lot of contributors look at existing code rather then documentation.

I've seen a lot of projects get to the point where they end up not wanting to do anything about this, and people are dissuaded from any form of cleanup because no decision was made about any sort of preferred style. These codebases end up stagnating heavily with code getting worse and worse because there aren't efforts to keep it tidy. I very much like the approach the IDE has taken since VS2002, which is to always be willing to make safe and simple cleanup changes throughout the codebase toward keeping things continually healthy.

@CyrusNajmabadi - Sure, I understand you. I only meant to recommend separating out the cleanup from functional changes for things that have no agreed upon guidelines or team preferences at PR time. Once we have agreed upon guidelines, I think code style fixup should be done as part of functional changes for files/functions being touched. Ideally, once we have NuGet code style analyzers/code fixes for our rules, these will be enforced via build, but till then we need to find a good balance to ensure we are driving the PR (and hence the functional changes) towards completion and at the same time not letting the code get polluted with many different code styles. Hopefully, the guidelines will provide clarity.

@CyrusNajmabadi - Sure, I understand you. I only meant to recommend separating out the cleanup from functional changes for things that have no agreed upon guidelines or team preferences at PR time.

That's fine. It was a second commit after all :) Now if this needs to be a second PR... that's somewhat of a more aggressive change, which i think will vastly demotivate cleanup from happening. The reason for that is that it's much easier to just roll cleanup into larger work you're doing, whereas going through the entire process that involves a PR is asking a lot. This is esp. true as i expect cleanup PRs to be even lower pri for the team, which effectively means that they won't happen, or will happen so slowly that you'll just be juggling PRs even longer.

Once we have agreed upon guidelines,

This also worries me (a bit). We've been working on codebases for decades. I've seen how long it takes to actually get around to having guidelines and having those guidelines sufficient for all the types of changes that people want to make. This ties into my previous statement htat trying to go this route just seems to rarely bear fruit. Which is why the general approach the IDE has taken (which has actually worked) is to just let cleanup happen and to recognize that trying to make too much process around it is actually what kills it, and there's little issue with allowing it to happen organically.

Now, if you can actually great and maintain guidelines expeditiously, then that would change my opinion. Until that point, i would prefer hte status quo be kept. And once a new system has proven itself, we can delegate to that as the mechanism we prfer. However, holding off until that happens is something i would like to avoid at all costs.

@sharwell @mavasani feel free to merge.

@jinujoseph for approval

@jinujoseph can you approve/merge? Thanks!

Approved to merge for 15.8.preview2
The style changes pending design review

Note: style changes were rolled back. So this is fine to merge as is. @sharwell will be bringing up what style preferences should be recommended at a future point.