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

Format range #1043

Merged
merged 9 commits into from
Dec 11, 2017
Merged

Format range #1043

merged 9 commits into from
Dec 11, 2017

Conversation

akshita31
Copy link
Contributor

Issue : dotnet/vscode-csharp#214

On selecting after the first (alphanumeric) character in "Format Selection" , the code was not trimming the preceding white spaces in the first line of the selection. Modified the code to set the start of the span to the full span of the intersecting token and added the relevant test cases.

Please review : @DustinCampbell @TheRealPiotrP @rchande

Copy link

@rchande rchande left a comment

Choose a reason for hiding this comment

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

Looking pretty good, minor nits.

var text = await document.GetTextAsync();
var start = text.Lines.GetPosition(new LinePosition(request.Line, request.Column));
var end = text.Lines.GetPosition(new LinePosition(request.EndLine, request.EndColumn));
var changes = await FormattingWorker.GetFormattingChanges(document, start, end);
var tokenStart = document.GetSyntaxRootAsync().Result.FindToken(start).FullSpan.Start;
Copy link

Choose a reason for hiding this comment

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

await

@@ -26,11 +26,12 @@ public FormatRangeService(OmniSharpWorkspace workspace)
{
return null;
}

Copy link

Choose a reason for hiding this comment

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

Remove spaces

@@ -128,6 +128,85 @@ public void M ()
new LinePositionSpanTextChange { StartLine = 2, StartColumn = 30, EndLine = 3, EndColumn = 0, NewText = "\n " });
}


Copy link

Choose a reason for hiding this comment

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

Remove extra blanks

};

await AssertTextChanges(string.Join("\r\n", source),
new LinePositionSpanTextChange { StartLine = 3, StartColumn = 5, EndLine = 4, EndColumn = 7, NewText = "\n" });
Copy link

Choose a reason for hiding this comment

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

This is really hard to read. How hard would it be to make it so the text change got applied and you could just specify the actual expected text?

[Fact]
public async Task TextChangesOnStartingSpanAfterFirstCharacterInLine()
{
var source = new[]
Copy link

Choose a reason for hiding this comment

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

I think you are storing all of these as string arrays and immediately string.join ing them. Might as well use string literals...

public static IEnumerable<TextChange> GetTextChanges(SourceText oldText, IEnumerable<LinePositionSpanTextChange> changes)
{
var textChanges = new List<TextChange>();
foreach( var change in changes)
Copy link

Choose a reason for hiding this comment

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

extra space

textChanges.Add(new TextChange(span, newText));
}

return textChanges.OrderBy(change => change.Span);
Copy link

Choose a reason for hiding this comment

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

I don't think this is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes returned by the service are arranged in descending order and the WithChanges function that we are using to get the new text for comparison requires the changes to be arranged in ascending order that is why this line has been added.

Copy link

Choose a reason for hiding this comment

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

Have you tested that? I thought SourceText.WithChanges didn't care about the order.

Copy link

@rchande rchande Dec 7, 2017

Choose a reason for hiding this comment

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


await AssertTextChanges(string.Join("\r\n", source), expected);
}

Copy link

Choose a reason for hiding this comment

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

Extra blanks

@@ -347,7 +336,7 @@ private static void AssertFormatTargetKind(SyntaxKind kind, string input)
public static IEnumerable<TextChange> GetTextChanges(SourceText oldText, IEnumerable<LinePositionSpanTextChange> changes)
{
var textChanges = new List<TextChange>();
foreach( var change in changes)
foreach(var change in changes)
Copy link

Choose a reason for hiding this comment

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

foreach (var change ...)

Copy link

@rchande rchande left a comment

Choose a reason for hiding this comment

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

Looks good to me. Nice work!

Copy link
Contributor

@DustinCampbell DustinCampbell left a comment

Choose a reason for hiding this comment

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

Looks good to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants