diff --git a/src/Microsoft.SqlTools.ServiceLayer/Formatter/Impl/CommaSeparatedListFormatter.cs b/src/Microsoft.SqlTools.ServiceLayer/Formatter/Impl/CommaSeparatedListFormatter.cs index 6af6e3db..3cea4509 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/Formatter/Impl/CommaSeparatedListFormatter.cs +++ b/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; - int commaToken = -1; + bool foundWhitespaceTokenBeforeComma; + int commaToken = FindCommaToken(start, end, out foundWhitespaceTokenBeforeComma); - for (int i = start; i < end && HasToken(i); i++) - { - 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) + if (foundWhitespaceTokenBeforeComma) { 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); - AddReplacement(new Replacement(token.StartIndex, ",", "")); + ProcessCommaList(commaToken, end); } + } + + 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() : " "; - AddReplacement(new Replacement( - GetTokenData(end).StartIndex, - string.Empty, - newValue - )); + // Only handle this case if there is no whitespace before the comma. Otherwise can + // ignore as we'll process & squash whitespace + if (!foundWhitespaceTokenBeforeComma) + { + AddNewlineOrSpace(end); + } } else { - NormalizeWhitespace f = FormatterUtilities.NormalizeNewLinesOrCondenseToOneSpace; - if (PlaceEachElementOnNewLine) - { - f = FormatterUtilities.NormalizeNewLinesEnsureOneNewLineMinimum; - } - - for (int i = commaToken + 1; i < end; i++) - { - SimpleProcessToken(i, f); - } + ProcessWhitespaceInRange(commaToken, end); } - // do we need to place the comma before the next statement in the list? - if (FormatOptions.PlaceCommasBeforeNextStatement) + // Add the comma just before the next token + 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); - TokenData tok = GetTokenData(end); - AddReplacement(new Replacement(tok.StartIndex, "", ",")); + AddNewlineOrSpace(end); + } + 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; + } + } } diff --git a/src/Microsoft.SqlTools.ServiceLayer/Formatter/Impl/FormatterTokens.cs b/src/Microsoft.SqlTools.ServiceLayer/Formatter/Impl/FormatterTokens.cs index f8272a5a..c73c4880 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/Formatter/Impl/FormatterTokens.cs +++ b/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"); diff --git a/src/Microsoft.SqlTools.ServiceLayer/Formatter/Impl/FormatterVisitor.cs b/src/Microsoft.SqlTools.ServiceLayer/Formatter/Impl/FormatterVisitor.cs index 7ff83662..c8659e04 100644 --- a/src/Microsoft.SqlTools.ServiceLayer/Formatter/Impl/FormatterVisitor.cs +++ b/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) diff --git a/test/Microsoft.SqlTools.ServiceLayer.Test.Common/TestData/TSqlFormatter/BaselineFiles/CreateProcedure_CommaBeforeNext.sql b/test/Microsoft.SqlTools.ServiceLayer.Test.Common/TestData/TSqlFormatter/BaselineFiles/CreateProcedure_CommaBeforeNext.sql new file mode 100644 index 00000000..74b0780e --- /dev/null +++ b/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 diff --git a/test/Microsoft.SqlTools.ServiceLayer.Test.Common/TestData/TSqlFormatter/BaselineFiles/CreateProcedure_CommaSpaced.sql b/test/Microsoft.SqlTools.ServiceLayer.Test.Common/TestData/TSqlFormatter/BaselineFiles/CreateProcedure_CommaSpaced.sql new file mode 100644 index 00000000..07c38044 --- /dev/null +++ b/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 diff --git a/test/Microsoft.SqlTools.ServiceLayer.Test.Common/TestData/TSqlFormatter/TestFiles/CreateProcedure_CommaHandling1.sql b/test/Microsoft.SqlTools.ServiceLayer.Test.Common/TestData/TSqlFormatter/TestFiles/CreateProcedure_CommaHandling1.sql new file mode 100644 index 00000000..2d326c1f Binary files /dev/null and b/test/Microsoft.SqlTools.ServiceLayer.Test.Common/TestData/TSqlFormatter/TestFiles/CreateProcedure_CommaHandling1.sql differ diff --git a/test/Microsoft.SqlTools.ServiceLayer.Test.Common/TestData/TSqlFormatter/TestFiles/CreateProcedure_CommaHandling2.sql b/test/Microsoft.SqlTools.ServiceLayer.Test.Common/TestData/TSqlFormatter/TestFiles/CreateProcedure_CommaHandling2.sql new file mode 100644 index 00000000..c5ce517d --- /dev/null +++ b/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 diff --git a/test/Microsoft.SqlTools.ServiceLayer.Test/Formatter/CreateProcedureFormatterTests.cs b/test/Microsoft.SqlTools.ServiceLayer.Test/Formatter/CreateProcedureFormatterTests.cs index 1870e41b..96475f1d 100644 --- a/test/Microsoft.SqlTools.ServiceLayer.Test/Formatter/CreateProcedureFormatterTests.cs +++ b/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() {