From 4e4bb4dd425f9d2b8b94c34b0593289b9b11c05c Mon Sep 17 00:00:00 2001 From: Bela VanderVoort Date: Tue, 10 Oct 2023 08:35:43 -0500 Subject: [PATCH] Getting element access to break more consistent in long invocation chains. (#967) * notes * Getting element access to break more consistently with invocations. closes #956 * Getting properties breaking consistent with invocations * Fixing more edge cases * some self review --------- Co-authored-by: Lasath Fernando --- .../EditorConfig/EditorConfigParser.cs | 1 - Src/CSharpier.Cli/FormattingCache.cs | 1 - Src/CSharpier.Cli/IgnoreFile.cs | 2 - .../cs/MemberChain_ArraysConsistent.test | 15 +++ .../cs/MemberChain_PropertiesConsistent.test | 33 ++++++ ...tionExpressions.test => MemberChains.test} | 29 +++-- Src/CSharpier/DocTypes/Doc.cs | 5 + .../InvocationExpression.cs | 109 ++++++++---------- 8 files changed, 119 insertions(+), 76 deletions(-) create mode 100644 Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChain_ArraysConsistent.test create mode 100644 Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChain_PropertiesConsistent.test rename Src/CSharpier.Tests/FormattingTests/TestFiles/cs/{InvocationExpressions.test => MemberChains.test} (92%) diff --git a/Src/CSharpier.Cli/EditorConfig/EditorConfigParser.cs b/Src/CSharpier.Cli/EditorConfig/EditorConfigParser.cs index b609e37ac..124699c5e 100644 --- a/Src/CSharpier.Cli/EditorConfig/EditorConfigParser.cs +++ b/Src/CSharpier.Cli/EditorConfig/EditorConfigParser.cs @@ -70,7 +70,6 @@ IFileSystem fileSystem { var configFile = ConfigFileParser.Parse(potentialPath, fileSystem); - DebugLogger.Log(potentialPath); yield return configFile; if (configFile.IsRoot) { diff --git a/Src/CSharpier.Cli/FormattingCache.cs b/Src/CSharpier.Cli/FormattingCache.cs index d8418f34c..c49597b09 100644 --- a/Src/CSharpier.Cli/FormattingCache.cs +++ b/Src/CSharpier.Cli/FormattingCache.cs @@ -114,7 +114,6 @@ public bool CanSkipFormatting(FileToFormatInfo fileToFormatInfo) var currentHash = Hash(fileToFormatInfo.FileContents) + this.optionsHash; if (this.cacheDictionary.TryGetValue(fileToFormatInfo.Path, out var cachedHash)) { - DebugLogger.Log(fileToFormatInfo.Path + " " + currentHash + " " + cachedHash); if (currentHash == cachedHash) { return true; diff --git a/Src/CSharpier.Cli/IgnoreFile.cs b/Src/CSharpier.Cli/IgnoreFile.cs index 62e584df0..236fabb4c 100644 --- a/Src/CSharpier.Cli/IgnoreFile.cs +++ b/Src/CSharpier.Cli/IgnoreFile.cs @@ -46,8 +46,6 @@ public static async Task Create( CancellationToken cancellationToken ) { - DebugLogger.Log("Creating ignore file for " + baseDirectoryPath); - var ignore = new Ignore.Ignore(); foreach (var name in alwaysIgnored) diff --git a/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChain_ArraysConsistent.test b/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChain_ArraysConsistent.test new file mode 100644 index 000000000..c48758912 --- /dev/null +++ b/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChain_ArraysConsistent.test @@ -0,0 +1,15 @@ +var x = someLongNameField + .CallMethod____________________________________() + .AccessArray[1] + .Property_______________; + +var x = someLongNameField + .CallMethod____________________________________() + .CallMethod(1) + .Property_______________; + +new Action(AssertConfigurationIsValid) + .ShouldThrow() + .Errors[0] + .UnmappedPropertyNames[0] + .ShouldBe(nameof(Destination.Count)); diff --git a/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChain_PropertiesConsistent.test b/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChain_PropertiesConsistent.test new file mode 100644 index 000000000..2dda4e8e7 --- /dev/null +++ b/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChain_PropertiesConsistent.test @@ -0,0 +1,33 @@ +var someVariable = someObject + .Property + .CallMethod(someValue => someValue.SomeProperty == someOtherValue___________________________); + +var someVariable = someObject + .Property() + .CallMethod(someValue => someValue.SomeProperty == someOtherValue___________________________); + +var someVariable = someObject + .Property + .CallMethod(someValue => someValue.SomeProperty == someOtherValue___________________________) + .CallMethod(); + +var someVariable = someObject + .Property() + .CallMethod(someValue => someValue.SomeProperty == someOtherValue___________________________) + .CallMethod(); + +// TODO too hard to change this for now, will do it in https://github.com/belav/csharpier/issues/451 +var someVariable = this.Property.CallMethod( + someValue => someValue.SomeProperty == someOtherValue___________________________ +); + +var someVariable = this.Property() + .CallMethod(someValue => someValue.SomeProperty == someOtherValue___________________________); + +var someVariable = this.Property + .CallMethod(someValue => someValue.SomeProperty == someOtherValue___________________________) + .CallMethod(); + +var someVariable = this.Property() + .CallMethod(someValue => someValue.SomeProperty == someOtherValue___________________________) + .CallMethod(); diff --git a/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/InvocationExpressions.test b/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChains.test similarity index 92% rename from Src/CSharpier.Tests/FormattingTests/TestFiles/cs/InvocationExpressions.test rename to Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChains.test index a1ababd92..aa0d4309a 100644 --- a/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/InvocationExpressions.test +++ b/Src/CSharpier.Tests/FormattingTests/TestFiles/cs/MemberChains.test @@ -51,11 +51,13 @@ class ClassName .Where(o => someLongCondition__________________________) .Where(o => someLongCondition__________________________); - var someValue = someOtherValue!.Thing! + var someValue = someOtherValue! + .Thing! .Where(o => someLongCondition__________________________) .Where(o => someLongCondition__________________________); - var someValue = someOtherValue.Thing + var someValue = someOtherValue + .Thing .Where(o => someLongCondition__________________________) .Where(o => someLongCondition__________________________); @@ -81,7 +83,8 @@ class ClassName this.SomeProperty.Setup(o => longThing_______________________________________) ); - roleNames.Value + roleNames + .Value .Where(o => o.SomeProperty____________________________________) .Select(o => o.SomethingElse); @@ -110,9 +113,12 @@ class ClassName someParameter____________________________________ )!; - var someVariable = someObject.Property.CallMethod( - someValue => someValue.SomeProperty == someOtherValue___________________________________ - ); + var someVariable = someObject + .Property + .CallMethod( + someValue => + someValue.SomeProperty == someOtherValue___________________________________ + ); CallMethod( firstParameter____________________________, @@ -231,11 +237,18 @@ class ClassName ) .CallMethod__________________(); - someThing_______________________.Property + someThing_______________________ + .Property + .CallMethod__________________() + .CallMethod__________________(); + + someThing_______________________ + ?.Property .CallMethod__________________() .CallMethod__________________(); - someThing_______________________?.Property + someThing_______________________ + .Property! .CallMethod__________________() .CallMethod__________________(); } diff --git a/Src/CSharpier/DocTypes/Doc.cs b/Src/CSharpier/DocTypes/Doc.cs index 1ef6eb456..199cef6a3 100644 --- a/Src/CSharpier/DocTypes/Doc.cs +++ b/Src/CSharpier/DocTypes/Doc.cs @@ -2,6 +2,11 @@ namespace CSharpier.DocTypes; internal abstract class Doc { + public string Print() + { + return DocPrinter.DocPrinter.Print(this, new PrinterOptions(), Environment.NewLine); + } + public override string ToString() { return DocSerializer.Serialize(this); diff --git a/Src/CSharpier/SyntaxPrinter/SyntaxNodePrinters/InvocationExpression.cs b/Src/CSharpier/SyntaxPrinter/SyntaxNodePrinters/InvocationExpression.cs index 5af38ff2f..d92b86f1c 100644 --- a/Src/CSharpier/SyntaxPrinter/SyntaxNodePrinters/InvocationExpression.cs +++ b/Src/CSharpier/SyntaxPrinter/SyntaxNodePrinters/InvocationExpression.cs @@ -116,6 +116,16 @@ And we want to work with them from Left to Right ) ); } + else if (expression is ElementAccessExpressionSyntax elementAccessExpression) + { + FlattenAndPrintNodes(elementAccessExpression.Expression, printedNodes, context); + printedNodes.Add( + new PrintedNode( + elementAccessExpression, + Node.Print(elementAccessExpression.ArgumentList, context) + ) + ); + } else if (expression is MemberAccessExpressionSyntax memberAccessExpressionSyntax) { FlattenAndPrintNodes(memberAccessExpressionSyntax.Expression, printedNodes, context); @@ -169,8 +179,6 @@ expression is PostfixUnaryExpressionSyntax } } - // TODO maybe this should work more like prettier, where it makes groups in a way that they try to fill lines - // TODO also prettier doesn't seem to use fluid private static List> GroupPrintedNodesOnLines(List printedNodes) { // We want to group the printed nodes in the following manner @@ -223,25 +231,22 @@ List printedNodes // will be grouped as // [ // [Identifier, InvocationExpression], - // [MemberAccessExpression, MemberAccessExpression, InvocationExpression], + // [MemberAccessExpression] + // [MemberAccessExpression, InvocationExpression], // [MemberAccessExpression, InvocationExpression], // [MemberAccessExpression], // ] // so that we can print it as // a() - // .b.c() + // .b + // .c() // .d() // .e - // The first group is the first node followed by - // - as many InvocationExpression as possible - // < fn()()() >.something() - // - as many array accessors as possible - // < fn()[0][1][2] >.something() - // - then, as many MemberAccessExpression as possible but the last one - // < this.items >.something() - + // TODO #451 this whole thing could possibly just turn into a big loop + // based on the current node, and the next/previous node, decide when to create new groups. + // certain nodes need to stay in the current group, other nodes indicate that a new group needs to be created. var groups = new List>(); var currentGroup = new List { printedNodes[0] }; var index = 1; @@ -257,63 +262,47 @@ List printedNodes } } - if (printedNodes[0].Node is not InvocationExpressionSyntax) + if ( + printedNodes[0].Node is not (InvocationExpressionSyntax or PostfixUnaryExpressionSyntax) + && index < printedNodes.Count + && printedNodes[index].Node + is ElementAccessExpressionSyntax + or PostfixUnaryExpressionSyntax + ) { - for (; index + 1 < printedNodes.Count; ++index) - { - /* this handles the special case where we want ?.Property on the same line - someThing_______________________?.Property - .CallMethod__________________() - .CallMethod__________________(); - */ - if ( - printedNodes[index].Node is ConditionalAccessExpressionSyntax - && printedNodes[index + 1].Node - is MemberBindingExpressionSyntax { Parent: MemberAccessExpressionSyntax } - ) - { - currentGroup.Add(printedNodes[index]); - currentGroup.Add(printedNodes[index + 1]); - index++; - continue; - } - - if ( - ( - IsMemberish(printedNodes[index].Node) - && ( - IsMemberish(printedNodes[index + 1].Node) - || printedNodes[index + 1].Node is PostfixUnaryExpressionSyntax - ) - ) - || printedNodes[index].Node is PostfixUnaryExpressionSyntax - ) - { - currentGroup.Add(printedNodes[index]); - } - else - { - break; - } - } + currentGroup.Add(printedNodes[index]); + index++; } groups.Add(currentGroup); currentGroup = new List(); - var hasSeenInvocationExpression = false; + var hasSeenNodeThatRequiresBreak = false; for (; index < printedNodes.Count; index++) { - if (hasSeenInvocationExpression && IsMemberish(printedNodes[index].Node)) + if ( + hasSeenNodeThatRequiresBreak + && printedNodes[index].Node + is MemberAccessExpressionSyntax + or ConditionalAccessExpressionSyntax + ) { groups.Add(currentGroup); currentGroup = new List(); - hasSeenInvocationExpression = false; + hasSeenNodeThatRequiresBreak = false; } - if (printedNodes[index].Node is InvocationExpressionSyntax) + if ( + printedNodes[index].Node + is ( + InvocationExpressionSyntax + or MemberAccessExpressionSyntax + or ElementAccessExpressionSyntax + or MemberBindingExpressionSyntax + ) + ) { - hasSeenInvocationExpression = true; + hasSeenNodeThatRequiresBreak = true; } currentGroup.Add(printedNodes[index]); } @@ -326,11 +315,6 @@ printedNodes[index].Node is ConditionalAccessExpressionSyntax return groups; } - private static bool IsMemberish(CSharpSyntaxNode node) - { - return node is MemberAccessExpressionSyntax or ConditionalAccessExpressionSyntax; - } - private static Doc PrintIndentedGroup(ExpressionSyntax node, IList> groups) { if (groups.Count == 0) @@ -370,10 +354,7 @@ private static bool ShouldMergeFirstTwoGroups(List> groups) var firstNode = groups[0][0].Node; - if ( - firstNode is IdentifierNameSyntax identifierNameSyntax - && identifierNameSyntax.Identifier.Text.Length <= 4 - ) + if (firstNode is IdentifierNameSyntax { Identifier.Text.Length: <= 4 }) { return true; }