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

Vertically Align Curly Braces #423

Closed
Kurt-von-Laven opened this issue Aug 25, 2021 · 5 comments · Fixed by #441
Closed

Vertically Align Curly Braces #423

Kurt-von-Laven opened this issue Aug 25, 2021 · 5 comments · Fixed by #441
Assignees
Milestone

Comments

@Kurt-von-Laven
Copy link
Contributor

Kurt-von-Laven commented Aug 25, 2021

This is just my opinion, so I am curious how others feel. CSharpier is already very nearly compatible with ReSharper's CleanupCode, and the two tools have the potential to complement each other nicely. The proposed change would both ensure that curly braces on separate lines are always vertically aligned (I think), and also bring CSharpier one step closer to being compatible with CleanupCode. CleanupCode offers extremely fine-grained configurability options covering nearly every well-known curly brace style, but not K & R curly braces only on lines containing only a close parenthesis.

Input:

if (SomeReallyLongParentheticalExceedsThePrintWidthCausingCSharpierToWrapTheLine())
{
    ThisIfBlockContains();
    MoreThanOneLine();
}

Current Output:

if (
    SomeReallyLongParentheticalExceedsThePrintWidthCausingCSharpierToWrapTheLine()
) {
    ThisIfBlockContains();
    MoreThanOneLine();
}

Desired Output:

if (
    SomeReallyLongParentheticalExceedsThePrintWidthCausingCSharpierToWrapTheLine()
)
{
    ThisIfBlockContains();
    MoreThanOneLine();
}
@belav
Copy link
Owner

belav commented Sep 14, 2021

There was a little bit of discussion on this previously in #144, and #34. I refer to the current output as SpaceBrace, I don't know that it has an official name.
I mainly went with SpaceBrace to keep things consistent with how prettier formats js/ts and because at that point no one had really objected to it. As far as I can tell it came from the airbnb styleguide

One concern I have with trying to keep CSharpier compatible with other tools that format code is that it may be never-ending. How many other tweaks would need to be made to get CSharpier compatible with CleanupCode? I don't think this should be the only reason for making a change, but in this case I think there are other reasons to consider moving away from SpaceBrace.

If no other c# formatting tools support SpaceBrace, then it is probably not a common way to format c#. Is SpaceBrace going to turn off a lot of developers who aren't used to seeing it? SpaceBrace does save a line, but is it actually any better? I personally prefer it, but that might just be because I am used to it.

Does it make sense to try to keep csharpier somewhat consistent with prettier? My day job involves a mix of TypeScript and C#, so I like it being somewhat consistent. But there are other areas where they differ, the big one is that csharpier breaks before binary operators.
I think that if there isn't an established standard for something in c#, and there is no strong argument for not doing it, then csharpier should strive to be consistent with prettier.

All of that being said, I am still torn on if csharpier should stick with SpaceBrace, switch to what you have suggested (BreakBreakBrace?), or go with this 3rd option (BreakBrace?) which is what I think I have seen most often in c# code.

if (
    SomeReallyLongParentheticalExceedsThePrintWidthCausingCSharpierToWrapTheLine())
{
    ThisIfBlockContains();
    MoreThanOneLine();
}

@Strepto
Copy link
Contributor

Strepto commented Sep 14, 2021

Adding a data-point here for discussion. I have no merit to my opinion so feel free to disregard it! I'm using the terminology from the post above.

I can not remember seeing space-brace in C# code. As far as I know the BreakBrace is the most used, and the most common for Microsoft repos, as seen in roslyn and runtime (links are to arbitrarily picked examples).

Prettier uses braces on the end of line (if (true) {) , so if keeping the SpaceBrace is more consistent in prettier I would consider using the same style for if (true) { in all c# statement blocks but that is an "all-in" approach which is not consistent with any common dotnet standard.

I think my personal "most readable formatting" idea leans towards breakbreakbrace, as the OP suggests.

@Kurt-von-Laven
Copy link
Contributor Author

One concern I have with trying to keep CSharpier compatible with other tools that format code is that it may be never-ending. How many other tweaks would need to be made to get CSharpier compatible with CleanupCode? I don't think this should be the only reason for making a change, but in this case I think there are other reasons to consider moving away from SpaceBrace.

Totally valid concern. I have only looked into this on one code base, so I can't say definitively, but I believe the literal answer to your question may be none whatsoever. CleanupCode offers a vast compendium of extremely fine-grained settings, and a few of them like this one would need to be changed from their default values.

My two cents would be that your best bet is to go with the flow in terms of what the community is doing in this case. My instinct is that would help keep the friction as low as possible for adopting this tool. Personally, I would prefer any of the options provided by CleanupCode, and beyond that have few opinions.

@belav belav added this to the Beta milestone Sep 18, 2021
belav added a commit that referenced this issue Sep 20, 2021
@belav belav modified the milestones: Beta, 0.9.10 Sep 20, 2021
@belav belav self-assigned this Sep 20, 2021
belav added a commit that referenced this issue Sep 20, 2021
belav added a commit that referenced this issue Sep 25, 2021
* Moving away from SpaceBrace

closes #423

* A little cleanup

Co-authored-by: Lasath Fernando <devel@lasath.org>
@Kurt-von-Laven
Copy link
Contributor Author

This may already be on your radar, @belav, but I think this change implies a change to the README: https://github.com/belav/csharpier#after.

belav added a commit that referenced this issue Sep 26, 2021
@belav
Copy link
Owner

belav commented Sep 27, 2021

This may already be on your radar, @belav, but I think this change implies a change to the README: https://github.com/belav/csharpier#after.

It will need a change and I didn't have it on my checklist until now. Thanks!

belav added a commit that referenced this issue Sep 27, 2021
belav added a commit that referenced this issue Oct 11, 2021
belav added a commit that referenced this issue Oct 18, 2021
* Cleaning up the doc tree

* Code review changes. Adding way to hide nulls on doc tree. Some minor performance tweaks

* Formatting files

* Moving away from SpaceBrace

closes #423

* A little cleanup

* Fixing extra indent in conditionals

Closes #7

* Cleaning up the doc tree

* Making some progress on chained invocations

Fixing bug with ConditionalGroup

* Switch back to printedNode

* A little progress, possibly backwards

* Possibly forwards this time

* Testing notes

* Merge predefinedTypes

* Changing my mind again

* Baby step forward

* Another baby step

* Fixing issues with ConditionalAccessExpression

* Fix case where chain contains a hardline

* Fixing comment edge case

* Another edge case

* more notes

* Merge when identifier is short

* Handling more edge cases

* Some cleanup

* Fixing edge case + some cleanup

* Review changes + more efficient DocSerializer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants