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

Retain single leading space when appending string in TypeLookUp #1064

Merged
merged 11 commits into from
Jan 9, 2018
Original file line number Diff line number Diff line change
Expand Up @@ -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)));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

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

private static string RetainSingleLeadingSpace(string input)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would TrimStartRetainingSingleLeadingSpace be clearer in terms of the intent?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: char instead of Char.

return input;
int offset = input.TakeWhile(c => char.IsWhiteSpace(c)).Count();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(It's faster).

return " " + input.Substring(offset);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()}";

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

class DocumentationItemBuilder
Expand Down
21 changes: 19 additions & 2 deletions tests/OmniSharp.Roslyn.CSharp.Tests/TypeLookupFacts.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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.";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we adding extra whitespace here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

Assert.Equal(expected, response.StructuredDocumentation.SummaryText);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did we eliminate these spaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@"This sample shows how to call the TestClass.GetZero method.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we adding extra whitespace here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


class TestClass
{
Expand Down Expand Up @@ -629,5 +629,22 @@ class testissue
@"Returns an array of type T .";
Assert.Equal(expectedReturns, response.StructuredDocumentation.ReturnsText);
}

[Fact]
public async Task StructuredDocumentationSpaceBeforeText()
{
string content = @"
public class TestClass
{
/// <summary><c>DoWork</c> is a method in the <c>TestClass</c> class.</summary>
public static void Do$$Work(int Int1)
{
}
}";
var response = await GetTypeLookUpResponse(content);
var expected =
@"DoWork is a method in the TestClass class.";
Assert.Equal(expected, response.StructuredDocumentation.SummaryText);
}
}
}