dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.92k stars 4.02k forks source link

"Wrap" refactorings should be a part of code-style #53464

Open jmarolf opened 3 years ago

jmarolf commented 3 years ago

According to telemetry our wrap parameter / argument refactorings are only invoked about 5% of the time they are shown (internal link here). Seems like these should just be code style options with the requirement that they auto-wrap after a configurable column length. I would suggest we set the default wrapping limit to 120 be default but have it be configurable.

alrz commented 3 years ago

Is there a tracking issue for "auto align" to be a part of code style? AFAIK there is no such formatting option at the moment.

Youssef1313 commented 3 years ago

I would suggest we set the default wrapping limit to 120 be default but have it be configurable.

This overlaps a little bit with https://github.com/dotnet/roslyn/issues/15406.

CyrusNajmabadi commented 3 years ago

Note: This is invoked more htan i would expect. However, i'm wary about inverting this to be a codestyle. This feature is intended very much for one-off, i just want to quickly adjust this one item that is currently hard to read.

I think the underlying tech is complimentary with having a code-style that can enforce this, but we still need tehse available for codebases (likely ours) that may not take on a column wrapping style choice, but still want to wrap things when appropriate.

CyrusNajmabadi commented 3 years ago

One thing that will likely help the numbers out more here is if users could opt-out of some of the wrapping options we show. For example, we support something like 9 different styles. But it's likely that some people will only use anywhere from 2-3 of those. The numbers would change a bunch here if we weren't showing items that the user will never use.

CyrusNajmabadi commented 3 years ago

Seriously, 4.9% seems way out of whack... 1 in 20 this list comes up, some changes wrapping. That's waaaaay higher than i would ever expect for a refactoring like this.

Cosifne commented 3 years ago

Conclusion:

Jon will start to investigate options

Editor guideline. (Draw a column line in the editor)

Analyzer

To tell the user a configurable maximum number of characters could be in this line. (There would be many problems, like Cref in comments, multiple comments)

RikkiGibson commented 2 years ago

I would like to have a code style option which enforces the relative indent depth of arguments:

// ok
MyMethod(MyArgument1, MyArgument2, MyArgument3);

// ok
MyMethod(MyArgument1,
    MyArgument2,
    MyArgument3);

// ok
MyMethod(
    MyArgument1,
    MyArgument2,
    MyArgument3);

// warning: fix formatting
MyMethod(MyArgument1,
         MyArgument2,
         MyArgument3);

Basically, the indent depth of arguments on subsequent lines should be 1 "tab" deeper in, rather than depending on how deep the ( of the call happens to be, which can change arbitrarily if the method being accessed is renamed, or if a more complex expression is used within the receiver (i.e. doing MyObject.MyMethod instead of just MyMethod).

I looked in the docs for this as well as searching issues on this repo. This is the most relevant issue I could find for it :) Apologies if this is something that was already considered and rejected separately.