Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add brace new line code style for switch expressions #36913

Closed
vsfeedback opened this issue Jul 1, 2019 · 24 comments · Fixed by #39396
Closed

Add brace new line code style for switch expressions #36913

vsfeedback opened this issue Jul 1, 2019 · 24 comments · Fixed by #39396
Assignees
Labels
Area-IDE Bug Developer Community The issue was originally reported on https://developercommunity.visualstudio.com IDE-Formatter Code formatter and/or smart indent Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Milestone

Comments

@vsfeedback
Copy link

Currently in Visual Studio 2019 release, there is no option to specify new line options for braces when using the new switch expression. Visual Studio will always place the opening brace on a new line, regardless of what has been specified for a switch statement, and there is no switch statement specific setting to change the option to having the opening brace on the same line. I typically change all of my code style options for all braces to begin on the same line as the statement/expression creating the brace, but this is one expression where I am unable to configure currently.

This issue has been moved from https://developercommunity.visualstudio.com/content/idea/524975/add-brace-new-line-code-style-for-switch-expressio.html
VSTS ticketId: 843465

These are the original issue comments:

Jane Wu [MSFT] on 4/10/2019, 00:52 AM (82 days ago):

Thank you for taking the time to provide your suggestion. We will do some preliminary checks to make sure we can proceed further. We'll provide an update once the issue has been triaged by the product team.

@jinujoseph jinujoseph added Area-IDE IDE-CodeStyle Built-in analyzers, fixes, and refactorings Bug help wanted The issue is "up for grabs" - add a comment if you are interested in working on it labels Jul 2, 2019
@jinujoseph jinujoseph added this to the Backlog milestone Jul 2, 2019
@jinujoseph jinujoseph added IDE-Formatter Code formatter and/or smart indent and removed IDE-CodeStyle Built-in analyzers, fixes, and refactorings labels Jul 2, 2019
@sharwell
Copy link
Member

sharwell commented Jul 2, 2019

The formatter should be using one of the existing options here:

public static Option<bool> NewLinesForBracesInControlBlocks { get; } = CreateNewLineForBracesOption(
NewLineOption.ControlBlocks, nameof(NewLinesForBracesInControlBlocks));

public static Option<bool> NewLinesForBracesInObjectCollectionArrayInitializers { get; } = CreateNewLineForBracesOption(
NewLineOption.ObjectCollectionsArrayInitializers, nameof(NewLinesForBracesInObjectCollectionArrayInitializers));

@TheJayMann
Copy link

I was the person to initially post this issue. Based on what you have shown, this option should be the one listed as Place open brace on new line for control blocks. It appears that it still isn't working for me. With the option enabled, I get this:

            var msg = 2 switch
            {
                1 => "One",
                2 => "Two",
                _ => "Not one or two"
            };
            for (var a = 1; a < 1; a++)
            {

            }

With the option disabled, I get this:

            var msg = 2 switch
            {
                1 => "One",
                2 => "Two",
                _ => "Not one or two"
            };
            for (var a = 1; a < 1; a++) {

            }

Currently, I am using .NET Core 3.0, CSharp 8.0, and Visual Studio 16.3.0 (I haven't had time to update to 16.3.1 yet).

@CyrusNajmabadi
Copy link
Member

I was the person to initially post this issue. Based on what you have shown, this option should be the one listed as Place open brace on new line for control blocks. It appears that it still isn't working for me.

Yes. The issue has not been fixed.

@tiesmaster
Copy link
Contributor

@sharwell @CyrusNajmabadi I'm going to take a stab at this one, let me know, if I shouldn't for some reason. I'm going to report back here, if I get stuck, or have questions.

@sharwell
Copy link
Member

@tiesmaster It should be fine to implement this. There is a chance the specific option changes following a design review, but it should be easy to update the implementation and tests to match regardless.

@sharwell sharwell added the Need Design Review The end user experience design needs to be reviewed and approved. label Oct 14, 2019
@CyrusNajmabadi
Copy link
Member

Good luck! Happy to help out if you need it :)

@tiesmaster
Copy link
Contributor

@CyrusNajmabadi Awesome!! Thnx, I'm probably going to need it ;)

I've managed to reproduce the bug, but so far, I haven't been able to get it fixed. I have this so far, but that doesn't "push the brace back".

So I need to do some more digging, but it's getting late, so I'm stopping for today. Just let me know if I'm on the right track (both tests, as the fixes).

@sharwell
Copy link
Member

Design review conclusion:

  • For fixing this issue (Fix brace code style for switch expressions #39396), we should use NewLinesForBracesInObjectCollectionArrayInitializers.
  • We should rename NewLinesForBracesInObjectCollectionArrayInitializers to NewLinesForBracesInExpressions, and update the Tools → Options and .editorconfig strings to reflect this change. @tiesmaster has the option of implementing this in Fix brace code style for switch expressions #39396, or we can implement it separately.
  • We should verify that pattern matching braces are also controlled by NewLinesForBracesInExpressions.

@sharwell sharwell added 4 - In Review A fix for the issue is submitted for review. and removed help wanted The issue is "up for grabs" - add a comment if you are interested in working on it Need Design Review The end user experience design needs to be reviewed and approved. labels Oct 24, 2019
@Chicken-Bones
Copy link

Chicken-Bones commented Feb 18, 2020

This formatting option should also cover CollectionInitializerExpression, ArrayInitializerExpression, ImplicitArrayCreationExpression and StackAllocArrayCreationExpression.

It currently inconsistently affects CollectionInitializerExpression where it will preserve lines but not force spaces.

if (currentToken.IsKind(SyntaxKind.OpenBraceToken) && currentToken.Parent != null && currentToken.Parent.IsKind(SyntaxKind.ObjectInitializerExpression))

vs

currentToken.Parent.Kind() == SyntaxKind.CollectionInitializerExpression))

@ValorMorgan
Copy link

Bump
Has there been any new developments on this? I see the conversation trailed off around November, 2019 with a remark in February. It seems there's also a lot of duplicate issues pointing back to this one.

@CyrusNajmabadi
Copy link
Member

Has there been any new developments on this?

We took this to a design meeting and decided in the design and behavior we want here. The issue is currently 'up for grabs' for anyone to fix, although @tiesmaster Expressed interest in doing that work.

We would take a well written pr that implements the design specified. Thanks!

@HappyNomad
Copy link

Hey @tiesmaster, are you still up for it? Otherwise, how about the product owners take care of it?

@CyrusNajmabadi
Copy link
Member

@ChainReactive We haven't heard from @tiesmaster in some time. Would you be interested in taking a look and contributing a PR here?

@HappyNomad
Copy link

@CyrusNajmabadi Sorry, I'm too busy working on my own products. I can't afford spending the time it'd take to learn Roslyn and contribute to it. Seems it'd be best if the product owners took responsibility.

@CyrusNajmabadi
Copy link
Member

Seems it'd be best if the product owners took responsibility.

We have responsibility for many issues. This is not more important than all the issues currently assigned to everyone currently. As this is a low impact issue, it likely won't get fixed until current critical work is done, or unless someone wants to take their spare time to fix it. :)

I can't afford spending the time it'd take to learn Roslyn and contribute to it

I'm happy to take my spare time to try to help you learn this :) We have many external contributers already, and you might like being able to be in a position to immediately address small paper cuts like this when they affect you instead of hoping that it might make the cut for someone else in teh future :)

@HappyNomad
Copy link

We have responsibility for many issues. This is not more important than all the issues currently assigned to everyone currently. As this is a low impact issue, it likely won't get fixed until current critical work is done, or unless someone wants to take their spare time to fix it. :)

Thanks for the explanation.

I'm happy to take my spare time to try to help you learn this :) We have many external contributers already, and you might like being able to be in a position to immediately address small paper cuts like this when they affect you instead of hoping that it might make the cut for someone else in teh future :)

There was a time in my life.... but not now. Good luck!

@sharwell
Copy link
Member

sharwell commented Sep 4, 2020

@tiesmaster @ChainReactive @CyrusNajmabadi I was the one supposed to finalize #39396 for merge. I'll update it now so this should already be fixed.

@tiesmaster
Copy link
Contributor

@here Indeed, as @sharwell indicated, I already put in a PR to fix this. I left that in an intermediate state, as there were still loose ends (or maybe just a single loose end, I'm not sure), so that was also a bit on me, I suppose.

@sharwell From all the other issues that point back to this issue, it's apparent that there is desire to get this in. Let me know if you need anything from me on the PR. Though, I'd need to dive into that again, if there is anything that needs to be change, so that would take a bit of time.

@CyrusNajmabadi Indeed, you haven't heard from me for some time. I started out with a new job, and together with some demanding things in my personal life (happy stuff, but still time consuming), I didn't have any time to continue with my contributing. I suppose I should have mentioned that, sorry about that.

I do like to start contributing again, but didn't really knew where to start. Getting help on certain issues would definitely help. I know the Gitter channel, but that didn't really work for me, somehow. I see there is Discord now, that might be a better fit for me. Perhaps having a dedicated person to ask questions to, would also help, something like a mentor.

@cremor
Copy link

cremor commented Sep 8, 2020

@sharwell Thanks for finishing the PR! I assume it is too late to get this in 16.8, right? Will it be in 16.9?

@sharwell
Copy link
Member

sharwell commented Sep 8, 2020

I think this will be 16.8 Preview 4.

@sharwell sharwell added Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented and removed 4 - In Review A fix for the issue is submitted for review. labels Sep 8, 2020
@sharwell sharwell modified the milestones: Backlog, Next Sep 8, 2020
@dibarbet dibarbet modified the milestones: Next, 16.8.P4 Sep 22, 2020
@joelhoisko
Copy link

Sorry for barging in late, but is this fix now up on some stable release? And what is the setting now available for the .editorconfig?
In this comment I figured that there is a object_collection_array_initalizers option, but I can't find any setting like that from the docs regarding new lines.

@sharwell
Copy link
Member

@joelhoisko The .editorconfig property is csharp_new_line_before_open_brace. When the value is all or includes object_collection_array_initializers, a new line will be used for switch expressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Bug Developer Community The issue was originally reported on https://developercommunity.visualstudio.com IDE-Formatter Code formatter and/or smart indent Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.