-
Notifications
You must be signed in to change notification settings - Fork 416
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
Changes from 6 commits
fd25efe
1c2ef95
a328b4d
b7782a9
ecd2bef
8a2f4df
f76d560
bfb18ec
c092873
b245a7c
0f5d3dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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))); | ||
} | ||
|
||
private static string GetCref(string cref) | ||
|
@@ -166,6 +166,16 @@ private static string GetCref(string cref) | |
} | ||
return cref + " "; | ||
} | ||
|
||
private static string RetainSingleLeadingSpace(string input) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would |
||
{ | ||
if (string.IsNullOrWhiteSpace(input)) | ||
return string.Empty; | ||
if (!Char.IsWhiteSpace(input[0])) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style: |
||
return input; | ||
int offset = input.TakeWhile(c => char.IsWhiteSpace(c)).Count(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (It's faster). |
||
return " " + input.Substring(offset); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()}"; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, we allocate one string fewer by not calling TrimStart. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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."; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are we adding extra whitespace here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above |
||
Assert.Equal(expected, response.StructuredDocumentation.SummaryText); | ||
} | ||
|
||
|
@@ -499,7 +499,7 @@ public class TestClass | |
}"; | ||
var response = await GetTypeLookUpResponse(content); | ||
var expected = | ||
@"This sample shows how to call the TestClass.GetZero method. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why did we eliminate these spaces? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we adding extra whitespace here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Initially |
||
|
||
class TestClass | ||
{ | ||
|
@@ -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); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.