Skip to content

Commit

Permalink
Format on type fixes (#1798)
Browse files Browse the repository at this point in the history 
* Undo changes

* Test fixes

* Increase timeout

* Remove double event listening

* Remove test

* Revert "Remove test"

This reverts commit e240c3f.

* Revert "Remove double event listening"

This reverts commit af573be.

* #1096 The if statement is automatically formatted incorrectly

* Merge fix

* Add more tests

* More tests

* Typo

* Test

* Also better handle multiline arguments

* Add a couple missing periods

[skip ci]

* Undo changes

* Test fixes

* Increase timeout

* Remove double event listening

* Remove test

* Revert "Remove test"

This reverts commit e240c3f.

* Revert "Remove double event listening"

This reverts commit af573be.

* Merge fix

* #1257 On type formatting errors for args and kwargs

* Handle f-strings

* Stop importing from test code

* #1308 Single line statements leading to an indentation on the next line

* #726 editing python after inline if statement invalid indent

* Undo change

* Move constant

* Harden LS startup error checks

* #1364 Intellisense doesn't work after specific const string

* Telemetry for the analysis enging

* PR feedback

* Fix typo

* Test baseline update

* Jedi 0.12

* Priority to goto_defition

* News

* Replace unzip

* Linux flavors + test

* Grammar check

* Grammar test

* Test baselines

* Add news

* Pin dependency

[skip ci]

* Specify markdown as preferable format

* Improve function argument detection

* Specify markdown

* Pythia setting

* Baseline updates

* Baseline update

* Improve startup

* Handle missing interpreter better

* Handle interpreter change

* Delete old file

* Fix LS startup time reporting

* Remove Async suffix from IFileSystem

* Remove Pythia

* Remove pre-packaged MSIL

* Exe name on Unix

* Plain linux

* Fix casing

* Fix message

* Update PTVS engine activation steps

* Type formatter eats space in from .

* fIX CASING

* Remove flag

* Don't wait for LS

* Small test fixes

* Update hover baselines

* Rename the engine

* Formatting 1

* Add support for 'rf' strings

* Add two spaces before comment per PEP

* Fix @ operator spacing

* Handle module and unary ops

* Type hints

* Fix typo

* Trailing comma

* Require space after if

* Update list of keywords

* PR feedback
  • Loading branch information
Mikhail Arkhipov authored May 31, 2018
1 parent 6d2d390 commit 51224e7
Show file tree
Hide file tree
Showing 6 changed files with 160 additions and 42 deletions.
83 changes: 64 additions & 19 deletions src/client/formatters/lineFormatter.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,21 @@ import { TextRangeCollection } from '../language/textRangeCollection';
import { Tokenizer } from '../language/tokenizer';
import { ITextRangeCollection, IToken, TokenType } from '../language/types';

const keywordsWithSpaceBeforeBrace = [
'and', 'as', 'assert', 'await',
'del',
'except', 'elif',
'for', 'from',
'global',
'if', 'import', 'in', 'is',
'lambda',
'nonlocal', 'not',
'or',
'raise', 'return',
'while', 'with',
'yield'
];

export class LineFormatter {
private builder = new TextBuilder();
private tokens: ITextRangeCollection<IToken> = new TextRangeCollection<IToken>([]);
Expand Down Expand Up @@ -59,7 +74,7 @@ export class LineFormatter {
}
const id = this.text.substring(t.start, t.end);
this.builder.append(id);
if (this.keywordWithSpaceAfter(id) && next && this.isOpenBraceType(next.type)) {
if (this.isKeywordWithSpaceBeforeBrace(id) && next && this.isOpenBraceType(next.type)) {
// for x in ()
this.builder.softAppendSpace();
}
Expand All @@ -75,9 +90,9 @@ export class LineFormatter {
break;

case TokenType.Comment:
// Add space before in-line comment.
// Add 2 spaces before in-line comment per PEP guidelines.
if (prev) {
this.builder.softAppendSpace();
this.builder.softAppendSpace(2);
}
this.builder.append(this.text.substring(t.start, t.end));
break;
Expand All @@ -98,28 +113,35 @@ export class LineFormatter {
private handleOperator(index: number): void {
const t = this.tokens.getItemAt(index);
const prev = index > 0 ? this.tokens.getItemAt(index - 1) : undefined;
const opCode = this.text.charCodeAt(t.start);
const next = index < this.tokens.count - 1 ? this.tokens.getItemAt(index + 1) : undefined;

if (t.length === 1) {
const opCode = this.text.charCodeAt(t.start);
switch (opCode) {
case Char.Equal:
if (this.handleEqual(t, index)) {
return;
}
break;
this.handleEqual(t, index);
return;
case Char.Period:
if (prev && this.isKeyword(prev, 'from')) {
this.builder.softAppendSpace();
}
this.builder.append(this.text[t.start]);
this.builder.append('.');
if (next && this.isKeyword(next, 'import')) {
this.builder.softAppendSpace();
}
return;
case Char.At:
if (prev) {
// Binary case
this.builder.softAppendSpace();
this.builder.append('@');
this.builder.softAppendSpace();
} else {
this.builder.append('@');
}
return;
case Char.ExclamationMark:
this.builder.append(this.text[t.start]);
this.builder.append('!');
return;
case Char.Asterisk:
if (prev && this.isKeyword(prev, 'lambda')) {
Expand Down Expand Up @@ -153,19 +175,34 @@ export class LineFormatter {

this.builder.softAppendSpace();
this.builder.append(this.text.substring(t.start, t.end));

// Check unary case
if (prev && prev.type === TokenType.Operator) {
if (opCode === Char.Hyphen || opCode === Char.Plus || opCode === Char.Tilde) {
return;
}
}
this.builder.softAppendSpace();
}

private handleEqual(t: IToken, index: number): boolean {
private handleEqual(t: IToken, index: number): void {
if (this.isMultipleStatements(index) && !this.braceCounter.isOpened(TokenType.OpenBrace)) {
return false; // x = 1; x, y = y, x
// x = 1; x, y = y, x
this.builder.softAppendSpace();
this.builder.append('=');
this.builder.softAppendSpace();
return;
}

// Check if this is = in function arguments. If so, do not add spaces around it.
if (this.isEqualsInsideArguments(index)) {
this.builder.append('=');
return true;
return;
}
return false;

this.builder.softAppendSpace();
this.builder.append('=');
this.builder.softAppendSpace();
}

private handleOther(t: IToken, index: number): void {
Expand All @@ -188,6 +225,12 @@ export class LineFormatter {
return;
}

if (t.type === TokenType.Number && prev && prev.type === TokenType.Operator && prev.length === 1 && this.text.charCodeAt(prev.start) === Char.Tilde) {
// Special case for ~ before numbers
this.builder.append(this.text.substring(t.start, t.end));
return;
}

if (t.type === TokenType.Unknown) {
this.handleUnknown(t);
} else {
Expand Down Expand Up @@ -224,6 +267,10 @@ export class LineFormatter {
return false;
}

if (index > 1 && this.tokens.getItemAt(index - 2).type === TokenType.Colon) {
return false; // Type hint should have spaces around like foo(x: int = 1) per PEP 8
}

const first = this.tokens.getItemAt(0);
if (first.type === TokenType.Comma) {
return true; // Line starts with commma
Expand Down Expand Up @@ -278,11 +325,9 @@ export class LineFormatter {
}
return false;
}
private keywordWithSpaceAfter(s: string): boolean {
return s === 'in' || s === 'return' || s === 'and' ||
s === 'or' || s === 'not' || s === 'from' ||
s === 'import' || s === 'except' || s === 'for' ||
s === 'as' || s === 'is';

private isKeywordWithSpaceBeforeBrace(s: string): boolean {
return keywordsWithSpaceBeforeBrace.indexOf(s) >= 0;
}
private isKeyword(t: IToken, keyword: string): boolean {
return t.type === TokenType.Identifier && t.length === keyword.length && this.text.substr(t.start, t.length) === keyword;
Expand Down
10 changes: 8 additions & 2 deletions src/client/language/textBuilder.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,14 @@ export class TextBuilder {
return this.segments.join('');
}

public softAppendSpace(): void {
if (!this.isLastWhiteSpace() && this.segments.length > 0) {
public softAppendSpace(count: number = 1): void {
if (this.segments.length === 0) {
return;
}
if (this.isLastWhiteSpace()) {
count = count - 1;
}
for (let i = 0; i < count; i += 1) {
this.segments.push(' ');
}
}
Expand Down
43 changes: 31 additions & 12 deletions src/client/language/tokenizer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,8 @@ export class Tokenizer implements ITokenizer {
case Char.Caret:
case Char.Equal:
case Char.ExclamationMark:
case Char.Percent:
case Char.Tilde:
length = nextChar === Char.Equal ? 2 : 1;
break;

Expand Down Expand Up @@ -350,23 +352,40 @@ export class Tokenizer implements ITokenizer {
this.tokens.push(new Token(TokenType.Comment, start, this.cs.position - start));
}

// tslint:disable-next-line:cyclomatic-complexity
private getStringPrefixLength(): number {
if (this.cs.currentChar === Char.f && (this.cs.nextChar === Char.SingleQuote || this.cs.nextChar === Char.DoubleQuote)) {
return 1; // f-string
if (this.cs.currentChar === Char.SingleQuote || this.cs.currentChar === Char.DoubleQuote) {
return 0; // Simple string, no prefix
}
if (this.cs.currentChar === Char.b || this.cs.currentChar === Char.B || this.cs.currentChar === Char.u || this.cs.currentChar === Char.U) {
if (this.cs.nextChar === Char.SingleQuote || this.cs.nextChar === Char.DoubleQuote) {
// b-string or u-string
return 1;

if (this.cs.nextChar === Char.SingleQuote || this.cs.nextChar === Char.DoubleQuote) {
switch (this.cs.currentChar) {
case Char.f:
case Char.F:
case Char.r:
case Char.R:
case Char.b:
case Char.B:
case Char.u:
case Char.U:
return 1; // single-char prefix like u"" or r""
default:
break;
}
if (this.cs.nextChar === Char.r || this.cs.nextChar === Char.R) {
// b-string or u-string with 'r' suffix
if (this.cs.lookAhead(2) === Char.SingleQuote || this.cs.lookAhead(2) === Char.DoubleQuote) {
return 2;
}
}

if (this.cs.lookAhead(2) === Char.SingleQuote || this.cs.lookAhead(2) === Char.DoubleQuote) {
const prefix = this.cs.getText().substr(this.cs.position, 2).toLowerCase();
switch (prefix) {
case 'rf':
case 'ur':
case 'br':
return 2;
default:
break;
}
}
return this.cs.currentChar === Char.SingleQuote || this.cs.currentChar === Char.DoubleQuote ? 0 : -1;
return -1;
}

private getQuoteType(): QuoteType {
Expand Down
33 changes: 32 additions & 1 deletion src/test/format/extension.lineFormatter.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ suite('Formatting - line formatter', () => {
testFormatLine('[ 1 :[2: (x,),y]]{1}', '[1:[2:(x,), y]]{1}');
});
test('Trailing comment', () => {
testFormatLine('x=1 # comment', 'x = 1 # comment');
testFormatLine('x=1 # comment', 'x = 1 # comment');
});
test('Single comment', () => {
testFormatLine('# comment', '# comment');
Expand Down Expand Up @@ -87,9 +87,14 @@ suite('Formatting - line formatter', () => {
});
test('Brace after keyword', () => {
testFormatLine('for x in(1,2,3)', 'for x in (1, 2, 3)');
testFormatLine('assert(1,2,3)', 'assert (1, 2, 3)');
testFormatLine('if (True|False)and(False/True)not (! x )', 'if (True | False) and (False / True) not (!x)');
testFormatLine('while (True|False)', 'while (True | False)');
testFormatLine('yield(a%b)', 'yield (a % b)');
});
test('Dot operator', () => {
testFormatLine('x.y', 'x.y');
testFormatLine('5 .y', '5.y');
});
test('Unknown tokens no space', () => {
testFormatLine('abc\\n\\', 'abc\\n\\');
Expand Down Expand Up @@ -121,6 +126,32 @@ suite('Formatting - line formatter', () => {
test('from..x import', () => {
testFormatLine('from..x import', 'from ..x import');
});
test('Raw strings', () => {
testFormatLine('z=r""', 'z = r""');
testFormatLine('z=rf""', 'z = rf""');
testFormatLine('z=R""', 'z = R""');
testFormatLine('z=RF""', 'z = RF""');
});
test('Binary @', () => {
testFormatLine('a@ b', 'a @ b');
});
test('Unary operators', () => {
testFormatLine('x= - y', 'x = -y');
testFormatLine('x= + y', 'x = +y');
testFormatLine('x= ~ y', 'x = ~y');
testFormatLine('x=-1', 'x = -1');
testFormatLine('x= +1', 'x = +1');
testFormatLine('x= ~1 ', 'x = ~1');
});
test('Equals with type hints', () => {
testFormatLine('def foo(x:int=3,x=100.)', 'def foo(x: int = 3, x=100.)');
});
test('Trailing comma', () => {
testFormatLine('a, =[1]', 'a, = [1]');
});
test('if()', () => {
testFormatLine('if(True) :', 'if (True):');
});
test('Grammar file', () => {
const content = fs.readFileSync(grammarFile).toString('utf8');
const lines = content.splitLines({ trim: false, removeEmptyEntries: false });
Expand Down
27 changes: 22 additions & 5 deletions src/test/language/tokenizer.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ suite('Language.Tokenizer', () => {
});
test('Unknown token', () => {
const t = new Tokenizer();
const tokens = t.tokenize('~$');
const tokens = t.tokenize('`$');
assert.equal(tokens.count, 1);

assert.equal(tokens.getItemAt(0).type, TokenType.Unknown);
Expand Down Expand Up @@ -301,20 +301,37 @@ suite('Language.Tokenizer', () => {
assert.equal(tokens.getItemAt(5).type, TokenType.Number);
assert.equal(tokens.getItemAt(5).length, 5);
});
test('Simple expression, leading minus', () => {
const t = new Tokenizer();
const tokens = t.tokenize('x == -y');
assert.equal(tokens.count, 4);

assert.equal(tokens.getItemAt(0).type, TokenType.Identifier);
assert.equal(tokens.getItemAt(0).length, 1);

assert.equal(tokens.getItemAt(1).type, TokenType.Operator);
assert.equal(tokens.getItemAt(1).length, 2);

assert.equal(tokens.getItemAt(2).type, TokenType.Operator);
assert.equal(tokens.getItemAt(2).length, 1);

assert.equal(tokens.getItemAt(3).type, TokenType.Identifier);
assert.equal(tokens.getItemAt(3).length, 1);
});
test('Operators', () => {
const text = '< <> << <<= ' +
'== != > >> >>= >= <=' +
'+ -' +
'+ - ~ %' +
'* ** / /= //=' +
'*= += -= **= ' +
'*= += -= ~= %= **= ' +
'& &= | |= ^ ^= ->';
const tokens = new Tokenizer().tokenize(text);
const lengths = [
1, 2, 2, 3,
2, 2, 1, 2, 3, 2, 2,
1, 1,
1, 1, 1, 1,
1, 2, 1, 2, 3,
2, 2, 2, 3,
2, 2, 2, 2, 2, 3,
1, 2, 1, 2, 1, 2, 2];
assert.equal(tokens.count, lengths.length);
for (let i = 0; i < tokens.count; i += 1) {
Expand Down
6 changes: 3 additions & 3 deletions src/test/pythonFiles/formatting/pythonGrammar.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ def test_eof_error(self):
compile(s, "<test>", "exec")
self.assertIn("unexpected EOF", str(cm.exception))

var_annot_global: int # a global annotated is necessary for test_var_annot
var_annot_global: int # a global annotated is necessary for test_var_annot

# custom namespace for testing __annotations__

Expand Down Expand Up @@ -643,7 +643,7 @@ def test_lambdef(self):
### lambdef: 'lambda' [varargslist] ':' test
l1 = lambda: 0
self.assertEqual(l1(), 0)
l2 = lambda: a[d] # XXX just testing the expression
l2 = lambda: a[d] # XXX just testing the expression
l3 = lambda: [2 < x for x in [-1, 3, 0]]
self.assertEqual(l3(), [0, 1, 0])
l4 = lambda x=lambda y=lambda z=1: z: y(): x()
Expand Down Expand Up @@ -1492,7 +1492,7 @@ def __imatmul__(self, o):
self.other = o
return self
m = M()
self.assertEqual(m@m, 4)
self.assertEqual(m @ m, 4)
m @= 42
self.assertEqual(m.other, 42)

Expand Down

0 comments on commit 51224e7

Please sign in to comment.