Skip to content

Allow quoted names in completions #18162

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.

Sign up for GitHub

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
3 commits merged into from
Sep 6, 2017
Merged

Allow quoted names in completions #18162

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ad1a5e8
Allow quoted names in completions
Aug 30, 2017
f28783e
Don't allow string literal completions if not in an object literal; a…
Aug 30, 2017
572cea6
Add TODO
Sep 6, 2017
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
14 changes: 11 additions & 3 deletions src/harness/fourslash.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -762,14 +762,18 @@ namespace FourSlash {
}
}

public verifyCompletionsAt(markerName: string, expected: string[]) {
public verifyCompletionsAt(markerName: string, expected: string[], options?: FourSlashInterface.CompletionsAtOptions) {
this.goToMarker(markerName);

const actualCompletions = this.getCompletionListAtCaret();
if (!actualCompletions) {
this.raiseError(`No completions at position '${this.currentCaretPosition}'.`);
}

if (options && options.isNewIdentifierLocation !== undefined && actualCompletions.isNewIdentifierLocation !== options.isNewIdentifierLocation) {
this.raiseError(`Expected 'isNewIdentifierLocation' to be ${options.isNewIdentifierLocation}, got ${actualCompletions.isNewIdentifierLocation}`);
}

const actual = actualCompletions.entries;

if (actual.length !== expected.length) {
Expand Down Expand Up @@ -3705,8 +3709,8 @@ namespace FourSlashInterface {
super(state);
}

public completionsAt(markerName: string, completions: string[]) {
this.state.verifyCompletionsAt(markerName, completions);
public completionsAt(markerName: string, completions: string[], options?: CompletionsAtOptions) {
this.state.verifyCompletionsAt(markerName, completions, options);
}

public quickInfoIs(expectedText: string, expectedDocumentation?: string) {
Expand Down Expand Up @@ -4314,4 +4318,8 @@ namespace FourSlashInterface {
actionName: string;
actionDescription: string;
}

export interface CompletionsAtOptions {
isNewIdentifierLocation?: boolean;
}
}
8 changes: 6 additions & 2 deletions src/services/completions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1635,13 +1635,17 @@ namespace ts.Completions {
// invalid identifier name. We need to check if whatever was inside the quotes is actually a valid identifier name.
// e.g "b a" is valid quoted name but when we strip off the quotes, it is invalid.
// We, thus, need to check if whatever was inside the quotes is actually a valid identifier name.
if (performCharacterChecks && !isIdentifierText(name, target)) {
return undefined;
if (performCharacterChecks && !isIdentifierText(name, target) && !isNumericLiteralText(name)) {
return JSON.stringify(name);
}

return name;
}

function isNumericLiteralText(name: string): boolean {
return /^-?\d+(\.\d+)?(e\d+)?$/.test(name);
Copy link
Member

Choose a reason for hiding this comment

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

There are contexts like property names:

            const foob = {
                -0() {
                    return 1;
                }
            };

where a leading minus would never be allowed (only an identifier, string literal, or number) - I don't think this should include the check for -??

Copy link
Member

Choose a reason for hiding this comment

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

Also, this helper probably should be somewhere central, since it will need to be updated once we support the numeric seperator proposal.

Copy link
Author

@ghost ghost Aug 30, 2017 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

We can just use a string literal in this case then, which makes this function unnecessary. 👍

}

// A cache of completion entries for keywords, these do not change between sessions
const _keywordCompletions: CompletionEntry[][] = [];
function getKeywordCompletions(keywordFilter: KeywordCompletionFilters): CompletionEntry[] {
Expand Down
11 changes: 2 additions & 9 deletions tests/cases/fourslash/completionForQuotedPropertyInPropertyAssignment1.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,5 @@
//// '/*1*/': ''
//// }

goTo.marker('0');
verify.completionListContains("jspm");
verify.completionListAllowsNewIdentifier();
verify.completionListCount(1);

goTo.marker('1');
verify.completionListContains("jspm:dev");
verify.completionListAllowsNewIdentifier();
verify.completionListCount(4);
verify.completionsAt("0", ["jspm", '"jspm:browser"', '"jspm:dev"', '"jspm:node"'], { isNewIdentifierLocation: true });
verify.completionsAt("1", ["jspm", "jspm:browser", "jspm:dev", "jspm:node"], { isNewIdentifierLocation: true });
11 changes: 2 additions & 9 deletions tests/cases/fourslash/completionForQuotedPropertyInPropertyAssignment2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,5 @@
//// }
//// }

goTo.marker('0');
verify.completionListContains("jspm");
verify.completionListAllowsNewIdentifier();
verify.completionListCount(1);

goTo.marker('1');
verify.completionListContains("jspm:dev");
verify.completionListAllowsNewIdentifier();
verify.completionListCount(4);
verify.completionsAt("0", ["jspm", '"jspm:browser"', '"jspm:dev"', '"jspm:node"'], { isNewIdentifierLocation: true });
verify.completionsAt("1", ["jspm", "jspm:browser", "jspm:dev", "jspm:node"], { isNewIdentifierLocation: true });
15 changes: 4 additions & 11 deletions tests/cases/fourslash/completionForQuotedPropertyInPropertyAssignment3.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,23 +4,16 @@
//// jspm: string;
//// 'jspm:browser': string;
//// } = {
//// /*0*/: "",
//// /*0*/: "",
//// }

//// let configFiles2: {
//// jspm: string;
//// 'jspm:browser': string;
//// } = {
//// jspm: "",
//// '/*1*/': ""
//// '/*1*/': ""
//// }

goTo.marker('0');
verify.completionListContains("jspm");
verify.completionListAllowsNewIdentifier();
verify.completionListCount(1);

goTo.marker('1');
verify.completionListContains("jspm:browser");
verify.completionListAllowsNewIdentifier();
verify.completionListCount(2);
verify.completionsAt("0", ["jspm", '"jspm:browser"'], { isNewIdentifierLocation: true });
verify.completionsAt("1", ["jspm", "jspm:browser"], { isNewIdentifierLocation: true });
11 changes: 1 addition & 10 deletions tests/cases/fourslash/completionListInvalidMemberNames.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,4 @@
////
////x./**/

goTo.marker();

verify.completionListContains("bar");
verify.completionListContains("break");
verify.completionListContains("any");
verify.completionListContains("$");
verify.completionListContains("b");

// Nothing else should show up
verify.completionListCount(5);
verify.completionsAt("", ['"foo "', "bar", "break", "any", '"#"', "$", "b", '"\u0031\u0062"']);
5 changes: 1 addition & 4 deletions tests/cases/fourslash/completionListInvalidMemberNames2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,4 @@
////}
////var x = Foo./**/

goTo.marker();
verify.completionListContains("X");
verify.completionListContains("Y");
verify.completionListCount(2);
verify.completionsAt("", ["X", "Y", '"☆"']);
10 changes: 2 additions & 8 deletions ...urslash/completion_enum-members-with-invalid-identifiers-should-not-show-in-completion.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,7 @@
//// a,
//// b
//// }
////
////
//// e./**/

goTo.marker();
verify.not.completionListContains('1');
verify.not.completionListContains('"1"');
verify.not.completionListContains('2');
verify.not.completionListContains('3');
verify.completionListContains('a');
verify.completionListContains('b');
verify.completionsAt("", ["1", "2", "3", "a", "b"]);
2 changes: 1 addition & 1 deletion tests/cases/fourslash/fourslash.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ declare namespace FourSlashInterface {
class verify extends verifyNegatable {
assertHasRanges(ranges: Range[]): void;
caretAtMarker(markerName?: string): void;
completionsAt(markerName: string, completions: string[]): void;
completionsAt(markerName: string, completions: string[], options?: { isNewIdentifierLocation?: boolean }): void;
indentationIs(numberOfSpaces: number): void;
indentationAtPositionIs(fileName: string, position: number, numberOfSpaces: number, indentStyle?: ts.IndentStyle, baseIndentSize?: number): void;
textAtCaretIs(text: string): void;
Expand Down