Skip to content
This repository was archived by the owner on Apr 14, 2022. It is now read-only.

On type formatting #126

Merged
merged 45 commits into from
Sep 25, 2018
Merged
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
4f3434b
Working line formatter based on VS Code implementation, without tests
jakebailey Sep 11, 2018
c52b83d
fix namespace change, rename file
jakebailey Sep 11, 2018
d8d1a74
move LineFormatter, add basic OnTypeFormatting tests, add LineFormatt…
jakebailey Sep 12, 2018
410b13f
mostly complete line formatter
jakebailey Sep 14, 2018
1a4c2db
rewrite formatter from scratch, fully passes existing tests with slig…
jakebailey Sep 17, 2018
cdbc45a
cleanup argument/lambda inside logic, remove leftover debugging state…
jakebailey Sep 17, 2018
32bae79
fix various formatting issues from vscode-python
jakebailey Sep 18, 2018
659075b
fix half-comment
jakebailey Sep 18, 2018
5ad26bc
add a few more unary operator tests for slicing
jakebailey Sep 18, 2018
17656b9
tweak test names, ellipsis test
jakebailey Sep 18, 2018
ca86bd3
test cleanups
jakebailey Sep 18, 2018
a506a8c
test unary ops inside function args
jakebailey Sep 18, 2018
c3ce7b9
test a few edge cases
jakebailey Sep 18, 2018
b956846
test against PEP448 unpacking, cleanup testing helpers, naming tweaks
jakebailey Sep 18, 2018
e5989df
remove stray comment
jakebailey Sep 18, 2018
ab1798d
simplify checking token kinds
jakebailey Sep 18, 2018
770aa4e
catch bad trigger character for OnTypeFormatting call
jakebailey Sep 18, 2018
b19ad65
instead of reading one extra line arbitrarily, tokenize ahead to the …
jakebailey Sep 18, 2018
eab9e6a
more consistent ArgumentNullException usage
jakebailey Sep 18, 2018
65842a2
remove async stuff from test cases
jakebailey Sep 18, 2018
a9f82ba
remove _reader member, document LineFormatter class/constructor
jakebailey Sep 18, 2018
a9d219b
documentation fixes
jakebailey Sep 18, 2018
9027d4b
check lines first, as most tokens won't span multiple lines
jakebailey Sep 18, 2018
5c00a04
fix multiline string assignment and function defaults/named args
jakebailey Sep 19, 2018
6630e87
simplify multiline string logic
jakebailey Sep 19, 2018
1fab534
directly test line continuations, fix commas inside brackets
jakebailey Sep 19, 2018
cae62a5
a few code analyzer fixes
jakebailey Sep 19, 2018
54556e3
simplify multiline string logic
jakebailey Sep 20, 2018
0892c5b
re-add soft space after multiline string
jakebailey Sep 20, 2018
73c6ff9
remove exception for unknown tokens, handle older python 2 features (…
jakebailey Sep 20, 2018
3c6a024
shuffle token get, fix AddToken typo
jakebailey Sep 20, 2018
e182904
explain non-comprehensive language server formatting tests
jakebailey Sep 24, 2018
37d1348
add context, dispose of server
jakebailey Sep 24, 2018
ee32c09
accessor formatting
jakebailey Sep 24, 2018
edaca3f
use Check class
jakebailey Sep 24, 2018
4df670f
fix failing test
jakebailey Sep 24, 2018
2e604ae
use DataRow/DataTestMethod style tests to show results for each
jakebailey Sep 24, 2018
4177069
fix missing DataTestMethod
jakebailey Sep 24, 2018
d2a6423
add a single string of whitespace instead of many single spaces
jakebailey Sep 24, 2018
96f3652
remove string specialization
jakebailey Sep 24, 2018
87d3392
move filtering into TokenizeLine
jakebailey Sep 24, 2018
94c18a0
simplify grammar file unchanged line checks
jakebailey Sep 24, 2018
f1cec62
remove test helpers, use server extension
jakebailey Sep 24, 2018
874a620
remove TextBuilder in favor of StringBuilder extension
jakebailey Sep 25, 2018
699b130
Merge remote-tracking branch 'upstream/master' into on-type-formatting
jakebailey Sep 25, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions src/Analysis/Engine/Impl/Infrastructure/TextBuilder.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
// Python Tools for Visual Studio
// Copyright(c) Microsoft Corporation
// All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the License); you may not use
// this file except in compliance with the License. You may obtain a copy of the
// License at http://www.apache.org/licenses/LICENSE-2.0
//
// THIS CODE IS PROVIDED ON AN *AS IS* BASIS, WITHOUT WARRANTIES OR CONDITIONS
// OF ANY KIND, EITHER EXPRESS OR IMPLIED, INCLUDING WITHOUT LIMITATION ANY
// IMPLIED WARRANTIES OR CONDITIONS OF TITLE, FITNESS FOR A PARTICULAR PURPOSE,
// MERCHANTABLITY OR NON-INFRINGEMENT.
//
// See the Apache Version 2.0 License for specific language governing
// permissions and limitations under the License.

using System.Collections.Generic;
using System.Linq;

namespace Microsoft.PythonTools.Analysis.Infrastructure {
internal class TextBuilder {
private readonly List<string> _segments = new List<string>();

public override string ToString() {
IEnumerable<string> seq = _segments;
if (IsLastWhitespace()) {
// Don't mutate when producing the string.
seq = _segments.Take(_segments.Count - 1);
}
return string.Concat(seq);
}

public void Append(string s) => _segments.Add(s);

public void Append(object o) => _segments.Add(o.ToString());

/// <summary>
/// "Soft" append a space to the text. If the most recent appended item
/// was whitespace, then nothing is added.
/// </summary>
/// <param name="count">How many spaces to append.</param>
/// <param name="allowLeading">True if spacing can be appended as the first item.</param>
public void SoftAppendSpace(int count = 1, bool allowLeading = false) {
if (_segments.Count == 0 && !allowLeading) {
return;
}

if (IsLastWhitespace()) {
count--;
}

for (var i = 0; i < count; i++) {
_segments.Add(" ");
}
}

private bool IsLastWhitespace() => _segments.Count != 0 && string.IsNullOrWhiteSpace(_segments.Last());
}
}
29 changes: 29 additions & 0 deletions src/Analysis/Engine/Test/LanguageServerTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -992,6 +992,19 @@ public async Task ParseAndAnalysisDiagnostics() {
);
}

Choose a reason for hiding this comment

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

This file is getting huge. Can we create LanguageServer folder and then separate FormattingTests file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Would you like me to do that in this PR, or a subsequent one? It might be more straightforward to make a PR to move all of the tests at once, since a lot of the "CreateServer" logic is shared between the tests.

[TestMethod, Priority(0)]
public async Task OnTypeFormatting() {
var s = await CreateServer();

var mod = await AddModule(s, "def foo ( ) :\n x = a + b\n x+= 1");
Copy link
Contributor

Choose a reason for hiding this comment

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

One of the good rules about the test is that the person who sees the test for the first time (but familiar with the code base) should be able to understand what's going on.

Here, without digging into implementation, I can only understand that you have created and initialized a Server instance and added a ProjectEntry to it with the content above. The rest is unclear.

There is no problem for a test to have some more lines of code, but it is important for a test to be as clear as possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you be more specific as to how you'd prefer it look? I'm not sure how this is unclear (or at least more unclear than tests like the Hover tests above it).

Choose a reason for hiding this comment

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

I personally would write most tests for the simplest object - line formatter itself and add just few that involve the LS itself as to verify the integration. This helps if someone wants to borrow the formatter but doesn't need the LS - tests then are easy to take with the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that's already how I have it set up. LineFormatterTests contains all of the test cases for the formatter itself. This test is just checking that the server does something. If you mean that I should remove any actual stylistic choices from here and do a simple check of "does the server return an edit instead of throwing NotImplemented", then I can do that too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I modified this test using the suggested added extension method to be more explicit, if you'd like to look again.


// Extended tests for line formatting are in LineFormatterTests.
// These just verify that the language server formats and returns something correct.
await AssertLineFormat(s, mod, 1, "\n", "def foo():", new SourceLocation(1, 1), new SourceLocation(1, 15));
await AssertLineFormat(s, mod, 2, "\n", "x = a + b", new SourceLocation(2, 5), new SourceLocation(2, 14));
await AssertLineFormat(s, mod, 3, "\n", "x += 1", new SourceLocation(3, 5), new SourceLocation(3, 10));
}

class GetAllExtensionProvider : ILanguageServerExtensionProvider {
public Task<ILanguageServerExtension> CreateAsync(IPythonLanguageServer server, IReadOnlyDictionary<string, object> properties, CancellationToken cancellationToken) {
return Task.FromResult<ILanguageServerExtension>(new GetAllExtension((Server)server, properties));
Expand Down Expand Up @@ -1185,6 +1198,22 @@ public static async Task AssertReferences(Server s, TextDocumentIdentifier docum
refs.Select(r => $"{r._kind ?? ReferenceKind.Reference};{r.range}").Should().Contain(contains).And.NotContain(excludes);
}

public static async Task AssertLineFormat(Server s, TextDocumentIdentifier document, int line, string ch, string newText, SourceLocation start, SourceLocation end) {
var edits = await s.DocumentOnTypeFormatting(new DocumentOnTypeFormattingParams {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have ServerExtension type that accumulates helper methods that help with simulating server activity. You can add SendDocumentOnTypeFormatting(Uri, Position, char) helper for it and call it directly from the test.

Copy link
Member Author

Choose a reason for hiding this comment

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

I implemented this, if you'd like to take a look.

textDocument = document,
ch = ch,
position = new SourceLocation(line+1, 1)
}, CancellationToken.None);

edits.Should().OnlyContain(new TextEdit {
newText = newText,
range = new Range {
start = start,
end = end
}
});
}

public Task<ILanguageServerExtension> CreateAsync(IPythonLanguageServer server, IReadOnlyDictionary<string, object> properties, CancellationToken cancellationToken) => throw new NotImplementedException();
}

Expand Down
Loading