Skip to content

Fix spacing of general (non-specific) tokens + tests #1222

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

Merged
merged 15 commits into from
Mar 28, 2018
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
51 changes: 41 additions & 10 deletions src/client/formatters/lineFormatter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,15 @@
import Char from 'typescript-char';
import { BraceCounter } from '../language/braceCounter';
import { TextBuilder } from '../language/textBuilder';
import { TextRangeCollection } from '../language/textRangeCollection';
import { Tokenizer } from '../language/tokenizer';
import { ITextRangeCollection, IToken, TokenType } from '../language/types';

export class LineFormatter {
private builder: TextBuilder;
private tokens: ITextRangeCollection<IToken>;
private braceCounter: BraceCounter;
private text: string;
private builder = new TextBuilder();
private tokens: ITextRangeCollection<IToken> = new TextRangeCollection<IToken>([]);
private braceCounter = new BraceCounter();
private text = '';

// tslint:disable-next-line:cyclomatic-complexity
public formatLine(text: string): string {
Expand Down Expand Up @@ -72,7 +73,7 @@ export class LineFormatter {
break;

default:
this.handleOther(t);
this.handleOther(t, i);
break;
}
}
Expand Down Expand Up @@ -108,24 +109,54 @@ export class LineFormatter {
if (this.braceCounter.isOpened(TokenType.OpenBrace)) {
// Check if this is = in function arguments. If so, do not
// add spaces around it.
const prev = this.tokens.getItemAt(index - 1);
const prevPrev = this.tokens.getItemAt(index - 2);
if (prev.type === TokenType.Identifier &&
(prevPrev.type === TokenType.Comma || prevPrev.type === TokenType.OpenBrace)) {
if (this.isEqualsInsideArguments(index)) {
this.builder.append('=');
return true;
}
}
return false;
}

private handleOther(t: IToken): void {
private handleOther(t: IToken, index: number): void {
if (this.isBraceType(t.type)) {
this.braceCounter.countBrace(t);
this.builder.append(this.text.substring(t.start, t.end));
return;
}

if (this.isEqualsInsideArguments(index - 1)) {
// Don't add space around = inside function arguments
this.builder.append(this.text.substring(t.start, t.end));
return;
}

if (index > 0) {
const prev = this.tokens.getItemAt(index - 1);
if (this.isOpenBraceType(prev.type)) {
// Don't insert space after (, [ or {
this.builder.append(this.text.substring(t.start, t.end));
return;
}
}

// In general, keep tokes separated
Copy link
Member

Choose a reason for hiding this comment

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

"tokens" and missing a period (couple of other comments are also missing periods).

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

private isEqualsInsideArguments(index: number): boolean {
if (index < 2) {
return false;
}
const prev = this.tokens.getItemAt(index - 1);
const prevPrev = this.tokens.getItemAt(index - 2);
if (prev.type === TokenType.Identifier &&
(prevPrev.type === TokenType.Comma || prevPrev.type === TokenType.OpenBrace)) {
return true;
}
return false;
}

private isOpenBraceType(type: TokenType): boolean {
return type === TokenType.OpenBrace || type === TokenType.OpenBracket || type === TokenType.OpenCurly;
}
Expand Down
17 changes: 16 additions & 1 deletion src/test/format/extension.onEnterFormat.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,23 @@ suite('Formatting - OnEnter provider', () => {
assert.equal(text, 'x.y', 'Line ending with period was reformatted');
});

test('Formatting line ending in string', async () => {
test('Formatting line with unknown neighboring tokens', async () => {
const text = await formatAtPosition(9, 0);
assert.equal(text, 'if x <= 1:', 'Line with unknown neighboring tokens was not formatted');
});

test('Formatting method definition with arguments', async () => {
const text = await formatAtPosition(10, 0);
assert.equal(text, 'def __init__(self, age=23)', 'Method definition with arguments was not formatted');
});

test('Formatting space after open brace', async () => {
const text = await formatAtPosition(11, 0);
assert.equal(text, 'while(1)', 'Method definition with arguments was not formatted');
Copy link
Member

Choose a reason for hiding this comment

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

This isn't a method definition so I think the comment of what failed isn't accurate.

});

test('Formatting line ending in string', async () => {
const text = await formatAtPosition(12, 0);
assert.equal(text, 'x + """', 'Line ending in multiline string was not formatted');
});

Expand Down
3 changes: 3 additions & 0 deletions src/test/pythonFiles/formatting/fileToFormatOnEnter.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,7 @@
x+1 #
@x
x.y
if x<=1:
Copy link
Member

Choose a reason for hiding this comment

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

This actually won't trigger the bug. It has to have a number on the left-hand side of the operator, e.g. if 1<=x:. Maybe worth keeping this one and just adding another test?

Copy link
Author

Choose a reason for hiding this comment

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

OK

Copy link
Author

Choose a reason for hiding this comment

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

Done

def __init__(self, age = 23)
while(1)
x+"""