Retain single leading space when appending string in TypeLookUp #1064

akshita31 · 2017-12-14T19:13:45Z

Before concatenating the string, a TrimStart was being called that removed all the whitespaces. Added a call to a function that retains a single space if the string begins with a whitespace. Added a test for the same and modified the existing tests to match the behaviour.

Please review : @DustinCampbell @TheRealPiotrP @rchande

…in text

jonsequitur · 2017-12-14T20:59:07Z

src/OmniSharp.Abstractions/Models/v1/TypeLookup/DocumentationComment.cs

@@ -166,6 +166,16 @@ private static string GetCref(string cref)
            }
            return cref + " ";
        }
+
+        private static string RetainSingleLeadingSpace(string input)


Would TrimStartRetainingSingleLeadingSpace be clearer in terms of the intent?

jonsequitur · 2017-12-14T21:02:32Z

src/OmniSharp.Abstractions/Models/v1/TypeLookup/DocumentationComment.cs

+            if (!Char.IsWhiteSpace(input[0]))
+                return input;
+            int offset = input.TakeWhile(c => char.IsWhiteSpace(c)).Count();
+            return " " + input.Substring(offset);


The use of LINQ caught my eye here. It looks like this will be a pretty hot path so we might do a little perf tuning.

Would deleting the LINQ expression and doing something like this satisfy the goal?

return $" {input.TrimStart()}";

FWIW, we allocate one string fewer by not calling TrimStart.

Oops, @jonsequitur is right--I don't know why I thought string.substring didn't allocate a string. I like his solution.

rchande

Please just use a for loop :).
Optionally experiment with inputs to string.split.

rchande · 2017-12-14T21:00:00Z

src/OmniSharp.Abstractions/Models/v1/TypeLookup/DocumentationComment.cs

+                return string.Empty;
+            if (!Char.IsWhiteSpace(input[0]))
+                return input;
+            int offset = input.TakeWhile(c => char.IsWhiteSpace(c)).Count();


I would just write this as a simple for loop instead of linq.

(It's faster).

rchande · 2017-12-14T21:02:45Z

src/OmniSharp.Abstractions/Models/v1/TypeLookup/DocumentationComment.cs

@@ -147,7 +147,7 @@ public static DocumentationComment From(string xmlDocumentation, string lineEndi
        private static string TrimMultiLineString(string input, string lineEnding)
        {
            var lines = input.Split(new string[] { "\n", "\r\n" }, StringSplitOptions.RemoveEmptyEntries);
-            return string.Join(lineEnding, lines.Select(l => l.TrimStart()));
+            return string.Join(lineEnding, lines.Select(l => RetainSingleLeadingSpace(l)));


What happens if you put (space space) in your list of strings to split on? I think that will give you strings that start with 0 or 1 spaces...

(Just spitballing, you don't have to do this)

I dont think that will work. Suppose the user separates two works with 3 spaces "hello hello1" , then these parts will also be split and joined with a lineEnding . That will be unnecessary splitting and incorrect joining.

TheRealPiotrP · 2017-12-14T21:24:14Z

tests/OmniSharp.Roslyn.CSharp.Tests/TypeLookupFacts.cs

@@ -499,7 +499,7 @@ public class TestClass
 }";
            var response = await GetTypeLookUpResponse(content);
            var expected =
-            @"This sample shows how to call the TestClass.GetZero method.


why did we eliminate these spaces?

That doesn't matter because the spaces were not a part of the string anyways. Just left-aligned it

TheRealPiotrP · 2017-12-14T21:25:46Z

tests/OmniSharp.Roslyn.CSharp.Tests/TypeLookupFacts.cs

@@ -499,7 +499,7 @@ public class TestClass
 }";
            var response = await GetTypeLookUpResponse(content);
            var expected =
-            @"This sample shows how to call the TestClass.GetZero method.
+@"This sample shows how to call the TestClass.GetZero  method.


Why are we adding extra whitespace here?

Initially return cref + " "; in GetCref was appending a space after TestClass.GetZero and the space before method was being trimmed so overall there was a single space. Now we are retaining the space before method as well, hence the output consists of 2 spaces. But this shouldn't be an issue as vscode collapses the multiple spaces in the text (as Hover API uses a MarkDown string)

TheRealPiotrP · 2017-12-14T21:26:07Z

tests/OmniSharp.Roslyn.CSharp.Tests/TypeLookupFacts.cs

@@ -452,7 +452,7 @@ public class TestClass
 }";
            var response = await GetTypeLookUpResponse(content);
            var expected =
-            @"DoWork is a method in the TestClass class. System.Console.WriteLine(System.String) for information about output statements.";
+            @"DoWork is a method in the TestClass class. System.Console.WriteLine(System.String)  for information about output statements.";


why are we adding extra whitespace here?

Same as above

rchande · 2018-01-03T18:41:38Z

src/OmniSharp.Abstractions/Models/v1/TypeLookup/DocumentationComment.cs

+        {
+            if (string.IsNullOrWhiteSpace(input))
+                return string.Empty;
+            if (!Char.IsWhiteSpace(input[0]))


Style: char instead of Char.

…p-roslyn into space_after_c

akshita31 added 6 commits December 13, 2017 14:27

Retaining a single space instead of removing all leading whitespaces …

fd25efe

…in text

Changing tests

1c2ef95

Changed tests

a328b4d

Resolved Merge Conflicts

b7782a9

Removed unnecessary function

ecd2bef

Renamed Method

8a2f4df

akshita31 changed the title ~~Retain Single leading space when appending string in TypeLookUp~~ Retain single leading space when appending string in TypeLookUp Dec 14, 2017

jonsequitur reviewed Dec 14, 2017

View reviewed changes

rchande suggested changes Dec 14, 2017

View reviewed changes

TheRealPiotrP reviewed Dec 14, 2017

View reviewed changes

Renamed Method and used space plus trimStart

f76d560

rchande approved these changes Jan 3, 2018

View reviewed changes

Ravi Chande and others added 4 commits January 3, 2018 10:41

Merge branch 'master' into space_after_c

bfb18ec

Replaced Char with char

c092873

Merge branch 'space_after_c' of https://github.com/akshita31/omnishar…

b245a7c

…p-roslyn into space_after_c

Merge branch 'master' into space_after_c

0f5d3dd

david-driscoll merged commit 55eadc0 into OmniSharp:master Jan 9, 2018

akshita31 mentioned this pull request Feb 1, 2018

Update to omnisharp 1.29 dotnet/vscode-csharp#1995

Merged

akshita31 deleted the space_after_c branch February 1, 2018 22:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retain single leading space when appending string in TypeLookUp #1064

Retain single leading space when appending string in TypeLookUp #1064

akshita31 commented Dec 14, 2017 •

edited

Loading

jonsequitur Dec 14, 2017

jonsequitur Dec 14, 2017

rchande Dec 14, 2017

rchande Dec 14, 2017

rchande left a comment

rchande Dec 14, 2017

rchande Dec 14, 2017

rchande Dec 14, 2017

rchande Dec 14, 2017

akshita31 Dec 14, 2017

TheRealPiotrP Dec 14, 2017

akshita31 Dec 14, 2017

TheRealPiotrP Dec 14, 2017

akshita31 Dec 14, 2017

TheRealPiotrP Dec 14, 2017

akshita31 Dec 14, 2017

rchande Jan 3, 2018

Retain single leading space when appending string in TypeLookUp #1064

Retain single leading space when appending string in TypeLookUp #1064

Conversation

akshita31 commented Dec 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchande left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshita31 commented Dec 14, 2017 •

edited

Loading