PlaceCommasBeforeNextStatement should handle correctly formatted text (#247)

- Fixed handling of PlaceCommasBeforeNextStatement so that it doesn't add whitespace unnecessarily. This ensures that repeated formats of the same text shouldn't keep adding whitespace
- Added tests covering a number of scenarios related to this

src/Microsoft.SqlTools.ServiceLayer/Formatter/Impl/CommaSeparatedListFormatter.cs

@@ -52,43 +52,15 @@ namespace Microsoft.SqlTools.ServiceLayer.Formatter
             int start = previousChild.Position.endTokenNumber;
             int end = nextChild.Position.startTokenNumber;
 
-            bool foundNonWhitespaceTokenBeforeComma = false;
+            bool foundWhitespaceTokenBeforeComma;
-            int commaToken = -1;
+            int commaToken = FindCommaToken(start, end, out foundWhitespaceTokenBeforeComma);
 
-            for (int i = start; i < end && HasToken(i); i++)
+            if (foundWhitespaceTokenBeforeComma)
-            {
-                TokenData td = GetTokenData(i);
-                if (td.TokenId == 44)
-                {
-                    commaToken = i;
-                    break;
-                }
-                else if (IsTokenWhitespace(td))
-                {
-                    foundNonWhitespaceTokenBeforeComma = true;
-                }
-            }
-
-            Debug.Assert(commaToken > -1, "No comma separating the children.");
-
-            if (foundNonWhitespaceTokenBeforeComma)
             {
                 ProcessTokenRange(start, commaToken);
             }
             else
             {
-
-#if DEBUG
-                for (int i = start; i < commaToken && HasToken(i); i++)
-                {
-                    TokenData td = GetTokenData(i);
-                    if (!IsTokenWhitespace(td))
-                    {
-                        Debug.Fail("unexpected token type of " + td.TokenId);
-                    }
-                }
-#endif
-                
                 // strip whitespace before comma
                 for (int i = start; i < commaToken; i++)
                 {
@@ -96,50 +68,105 @@ namespace Microsoft.SqlTools.ServiceLayer.Formatter
                 }
             }
 
-            // include comma after each element?
+            if (FormatOptions.PlaceCommasBeforeNextStatement)
-            if (!FormatOptions.PlaceCommasBeforeNextStatement)
             {
-                ProcessTokenRange(commaToken, commaToken + 1);
+                ProcessCommaListWithCommaMove(commaToken, end, foundWhitespaceTokenBeforeComma);
             }
             else
             {
-                TokenData token = GetTokenData(commaToken);
+                ProcessCommaList(commaToken, end);
-                AddReplacement(new Replacement(token.StartIndex, ",", ""));
             }
+        }
+
+        private void ProcessCommaListWithCommaMove(int commaToken, int end, bool foundWhitespaceTokenBeforeComma)
+        {
+            // Strip comma from current position
+            TokenData token = GetTokenData(commaToken);
+            AddReplacement(token.StartIndex, ",", string.Empty);
+
+            // Process text between comma and next token
 
             // special case if there is no white space between comma token and end of region
             if (commaToken + 1 == end)
             {
-                string newValue = PlaceEachElementOnNewLine ? Environment.NewLine + GetIndentString() : " ";
+                // Only handle this case if there is no whitespace before the comma. Otherwise can
-                AddReplacement(new Replacement(
+                // ignore as we'll process & squash whitespace
-                    GetTokenData(end).StartIndex,
+                if (!foundWhitespaceTokenBeforeComma)
-                    string.Empty,
+                {
-                    newValue
+                    AddNewlineOrSpace(end);
-                    ));
+                }
             }
             else
             {
-                NormalizeWhitespace f = FormatterUtilities.NormalizeNewLinesOrCondenseToOneSpace;
+                ProcessWhitespaceInRange(commaToken, end);
-                if (PlaceEachElementOnNewLine)
-                {
-                    f = FormatterUtilities.NormalizeNewLinesEnsureOneNewLineMinimum;
-                }
-
-                for (int i = commaToken + 1; i < end; i++)
-                {
-                    SimpleProcessToken(i, f);
-                }
 
             }
 
-            // do we need to place the comma before the next statement in the list?
+            // Add the comma just before the next token
-            if (FormatOptions.PlaceCommasBeforeNextStatement)
+            SimpleProcessToken(commaToken, FormatterUtilities.NormalizeNewLinesInWhitespace);
+            TokenData tok = GetTokenData(end);
+            AddReplacement(tok.StartIndex, string.Empty, ",");
+        }
+
+        private void ProcessCommaList(int commaToken, int end)
+        {
+            ProcessTokenRange(commaToken, commaToken + 1);
+
+            // special case if there is no white space between comma token and end of region
+            if (commaToken + 1 == end)
             {
-                SimpleProcessToken(commaToken, FormatterUtilities.NormalizeNewLinesInWhitespace);
+                AddNewlineOrSpace(end);
-                TokenData tok = GetTokenData(end);
+            }
-                AddReplacement(new Replacement(tok.StartIndex, "", ","));
+            else
+            {
+                ProcessWhitespaceInRange(commaToken, end);
             }
         }
-    }
 
+        private void ProcessWhitespaceInRange(int commaToken, int end)
+        {
+            NormalizeWhitespace f = FormatterUtilities.NormalizeNewLinesOrCondenseToOneSpace;
+            if (PlaceEachElementOnNewLine)
+            {
+                f = FormatterUtilities.NormalizeNewLinesEnsureOneNewLineMinimum;
+            }
+
+            for (int i = commaToken + 1; i <= end; i++)
+            {
+                SimpleProcessToken(i, f);
+            }
+        }
+
+        private void AddNewlineOrSpace(int end)
+        {
+            string newValue = PlaceEachElementOnNewLine ? Environment.NewLine + GetIndentString() : " ";
+            AddReplacement(
+                GetTokenData(end).StartIndex,
+                string.Empty,
+                newValue);
+        }
+
+        private int FindCommaToken(int start, int end, out bool foundWhitespaceTokenBeforeComma)
+        {
+            foundWhitespaceTokenBeforeComma = false;
+            int commaToken = -1;
+
+            for (int i = start; i < end && HasToken(i); i++)
+            {
+                TokenData td = GetTokenData(i);
+                if (td.TokenId == FormatterTokens.TOKEN_COMMA)
+                {
+                    commaToken = i;
+                    break;
+                }
+                else if (IsTokenWhitespace(td))
+                {
+                    foundWhitespaceTokenBeforeComma = true;
+                }
+            }
+
+            Debug.Assert(commaToken > -1, "No comma separating the children.");
+            return commaToken;
+        }
+    }
 }

src/Microsoft.SqlTools.ServiceLayer/Formatter/Impl/FormatterTokens.cs

@@ -22,6 +22,7 @@ namespace Microsoft.SqlTools.ServiceLayer.Formatter
             return (int)converter.ConvertFromString(tokenName);
         }
 
+        public static readonly int TOKEN_COMMA = 44;    // not included in Tokens enum for some reason
         public static readonly int TOKEN_FOR = ResolveTokenId("TOKEN_FOR");
         public static readonly int TOKEN_REPLICATION = ResolveTokenId("TOKEN_REPLICATION");
         public static readonly int TOKEN_ID = ResolveTokenId("TOKEN_ID");

src/Microsoft.SqlTools.ServiceLayer/Formatter/Impl/FormatterVisitor.cs

@@ -70,7 +70,7 @@ namespace Microsoft.SqlTools.ServiceLayer.Formatter
         internal static bool IsTokenWhitespaceOrComma(SqlScript script, int tokenIndex)
         {
             int tokenId = script.TokenManager.TokenList[tokenIndex].TokenId;
-            return script.TokenManager.IsTokenWhitespace(tokenId) || (tokenId == 44);
+            return script.TokenManager.IsTokenWhitespace(tokenId) || (tokenId == FormatterTokens.TOKEN_COMMA);
         }
 
         internal static bool IsTokenWhitespaceOrComment(SqlScript script, int tokenIndex)

test/Microsoft.SqlTools.ServiceLayer.Test.Common/TestData/TSqlFormatter/BaselineFiles/CreateProcedure_CommaBeforeNext.sql

@@ -0,0 +1,10 @@
+
+CREATE PROCEDURE [dbo].[sp1]
+AS
+SELECT
+    c1
+    ,c2
+    ,c3
+FROM
+    [dbo].[T1]
+RETURN 0

test/Microsoft.SqlTools.ServiceLayer.Test.Common/TestData/TSqlFormatter/BaselineFiles/CreateProcedure_CommaSpaced.sql

@@ -0,0 +1,6 @@
+
+CREATE PROCEDURE [dbo].[sp1]
+AS
+SELECT c1 ,c2 ,c3
+FROM [dbo].[T1]
+RETURN 0

test/Microsoft.SqlTools.ServiceLayer.Test.Common/TestData/TSqlFormatter/TestFiles/CreateProcedure_CommaHandling1.sql

@@ -0,0 +1,3 @@
+
+CREATE PROCEDURE [dbo].[sp1] AS SELECT c1, c2, c3 from [dbo].[T1] RETURN 0
+

test/Microsoft.SqlTools.ServiceLayer.Test.Common/TestData/TSqlFormatter/TestFiles/CreateProcedure_CommaHandling2.sql

@@ -0,0 +1,9 @@
+
+CREATE PROCEDURE [dbo].[sp1]
+AS
+SELECT
+    c1
+    ,c2, c3
+FROM
+    [dbo].[T1]
+RETURN 0

test/Microsoft.SqlTools.ServiceLayer.Test/Formatter/CreateProcedureFormatterTests.cs

@@ -66,6 +66,59 @@ namespace Microsoft.SqlTools.ServiceLayer.Test.Formatter
                 GetBaselineFile("CreateProcedure_Select.sql"), new FormatOptions(), true);
         }
 
+        [Fact]
+        public void CreateProcedure_CommaBeforeNextColumn()
+        {
+            // Verifies that commas are placed before the next column instead of after the current one,
+            // when PlaceCommasBeforeNextStatement = true
+            LoadAndFormatAndCompare("CreateProcedure_CommaBeforeNextColumn", 
+                GetInputFile("CreateProcedure_CommaHandling1.sql"),
+                GetBaselineFile("CreateProcedure_CommaBeforeNext.sql"),
+                new FormatOptions()
+                {
+                    DatatypeCasing = CasingOptions.Lowercase,
+                    KeywordCasing = CasingOptions.Uppercase,
+                    PlaceCommasBeforeNextStatement = true,
+                    PlaceEachReferenceOnNewLineInQueryStatements = true
+                }, true);
+        }
+
+        [Fact]
+        public void CreateProcedure_CommaBeforeNextColumnRepeated()
+        {
+            // Verifies that formatting isn't changed for text that's already been formatted
+            // as having the comma placed before the next column instead of after the prevous one
+            LoadAndFormatAndCompare("CreateProcedure_CommaBeforeNextColumnRepeated", 
+                GetInputFile("CreateProcedure_CommaHandling2.sql"),
+                GetBaselineFile("CreateProcedure_CommaBeforeNext.sql"),
+                new FormatOptions()
+                {
+                    DatatypeCasing = CasingOptions.Lowercase,
+                    KeywordCasing = CasingOptions.Uppercase,
+                    PlaceCommasBeforeNextStatement = true,
+                    PlaceEachReferenceOnNewLineInQueryStatements = true
+                },
+                true);
+        }
+        
+        [Fact]
+        public void CreateProcedure_CommaBeforeNextColumnNoNewline()
+        {
+            // Verifies that commas are placed before the next column instead of after the current one,
+            // when PlaceCommasBeforeNextStatement = true
+            // And that whitespace is used instead of newline if PlaceEachReferenceOnNewLineInQueryStatements = false
+            LoadAndFormatAndCompare("CreateProcedure_CommaBeforeNextColumnNoNewline",
+                GetInputFile("CreateProcedure_CommaHandling1.sql"),
+                GetBaselineFile("CreateProcedure_CommaSpaced.sql"),
+                new FormatOptions()
+                {
+                    DatatypeCasing = CasingOptions.Lowercase,
+                    KeywordCasing = CasingOptions.Uppercase,
+                    PlaceCommasBeforeNextStatement = true,
+                    PlaceEachReferenceOnNewLineInQueryStatements = false
+                }, true);
+        }
+
         [Fact]
         public void CreateProcedure_TwoPartName()
         {