Skip to content

Test for action description of code actions, and simplify description for extracting method to file #18030

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
4 commits merged into from
Aug 25, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 1 addition & 1 deletion src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -3696,7 +3696,7 @@
"code": 95003
},

"Extract function into '{0}'": {
"Extract function into {0}": {
"category": "Message",
"code": 95004
}
Expand Down
47 changes: 33 additions & 14 deletions src/harness/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2761,20 +2761,25 @@ namespace FourSlash {
});
}

public verifyRefactorAvailable(negative: boolean, name?: string, subName?: string) {
public verifyRefactorAvailable(negative: boolean, name: string, actionName?: string) {
const selection = this.getSelection();

let refactors = this.languageService.getApplicableRefactors(this.activeFile.fileName, selection) || [];
if (name) {
refactors = refactors.filter(r => r.name === name && (subName === undefined || r.actions.some(a => a.name === subName)));
}
refactors = refactors.filter(r => r.name === name && (actionName === undefined || r.actions.some(a => a.name === actionName)));
const isAvailable = refactors.length > 0;

if (negative && isAvailable) {
this.raiseError(`verifyApplicableRefactorAvailableForRange failed - expected no refactor but found some: ${refactors.map(r => r.name).join(", ")}`);
if (negative) {
if (isAvailable) {
this.raiseError(`verifyApplicableRefactorAvailableForRange failed - expected no refactor but found: ${refactors.map(r => r.name).join(", ")}`);
}
}
else if (!negative && !isAvailable) {
this.raiseError(`verifyApplicableRefactorAvailableForRange failed - expected a refactor but found none.`);
else {
if (!isAvailable) {
this.raiseError(`verifyApplicableRefactorAvailableForRange failed - expected a refactor but found none.`);
}
if (refactors.length > 1) {
this.raiseError(`2 available refactors both have name ${name} and action ${actionName}`);
Copy link
Member

Choose a reason for hiding this comment

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

Consider using the actual number.

}
}
}

Expand All @@ -2794,14 +2799,22 @@ namespace FourSlash {
}
}

public applyRefactor(refactorName: string, actionName: string) {
public applyRefactor({ refactorName, actionName, actionDescription }: FourSlashInterface.ApplyRefactorOptions) {
const range = this.getSelection();
const refactors = this.languageService.getApplicableRefactors(this.activeFile.fileName, range);
const refactor = ts.find(refactors, r => r.name === refactorName);
const refactor = refactors.find(r => r.name === refactorName);
if (!refactor) {
this.raiseError(`The expected refactor: ${refactorName} is not available at the marker location.`);
}

const action = refactor.actions.find(a => a.name === actionName);
if (!action) {
this.raiseError(`The expected action: ${action} is not included in: ${refactor.actions.map(a => a.name)}`);
}
if (action.description !== actionDescription) {
this.raiseError(`Expected action description to be ${JSON.stringify(actionDescription)}, got: ${JSON.stringify(action.description)}`);
}

const editInfo = this.languageService.getEditsForRefactor(this.activeFile.fileName, this.formatCodeSettings, range, refactorName, actionName);
for (const edit of editInfo.edits) {
this.applyEdits(edit.fileName, edit.textChanges, /*isFormattingEdit*/ false);
Expand Down Expand Up @@ -3682,8 +3695,8 @@ namespace FourSlashInterface {
this.state.verifyApplicableRefactorAvailableForRange(this.negative);
}

public refactorAvailable(name?: string, subName?: string) {
this.state.verifyRefactorAvailable(this.negative, name, subName);
public refactorAvailable(name: string, actionName?: string) {
this.state.verifyRefactorAvailable(this.negative, name, actionName);
}
}

Expand Down Expand Up @@ -4081,8 +4094,8 @@ namespace FourSlashInterface {
this.state.enableFormatting = false;
}

public applyRefactor(refactorName: string, actionName: string) {
this.state.applyRefactor(refactorName, actionName);
public applyRefactor(options: ApplyRefactorOptions) {
this.state.applyRefactor(options);
}
}

Expand Down Expand Up @@ -4295,4 +4308,10 @@ namespace FourSlashInterface {
return { classificationType, text, textSpan };
}
}

export interface ApplyRefactorOptions {
refactorName: string;
actionName: string;
actionDescription: string;
}
}
18 changes: 9 additions & 9 deletions src/services/refactors/extractMethod.ts
Original file line number Diff line number Diff line change
Expand Up @@ -560,32 +560,32 @@ namespace ts.refactor.extractMethod {
return "constructor";
case SyntaxKind.FunctionExpression:
return scope.name
? `function expression ${scope.name.getText()}`
? `function expression ${scope.name.text}`
: "anonymous function expression";
case SyntaxKind.FunctionDeclaration:
return `function ${scope.name.getText()}`;
return `function '${scope.name.text}'`;
case SyntaxKind.ArrowFunction:
return "arrow function";
case SyntaxKind.MethodDeclaration:
return `method ${scope.name.getText()}`;
return `method '${scope.name.getText()}`;
case SyntaxKind.GetAccessor:
return `get ${scope.name.getText()}`;
return `'get ${scope.name.getText()}'`;
case SyntaxKind.SetAccessor:
return `set ${scope.name.getText()}`;
return `'set ${scope.name.getText()}'`;
}
}
else if (isModuleBlock(scope)) {
return `namespace ${scope.parent.name.getText()}`;
return `namespace '${scope.parent.name.getText()}'`;
}
else if (isClassLike(scope)) {
return scope.kind === SyntaxKind.ClassDeclaration
? `class ${scope.name.text}`
? `class '${scope.name.text}'`
: scope.name.text
? `class expression ${scope.name.text}`
? `class expression '${scope.name.text}'`
: "anonymous class expression";
}
else if (isSourceFile(scope)) {
return `file '${scope.fileName}'`;
return "this file";
Copy link
Member

Choose a reason for hiding this comment

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

What about "file scope" or "top-level scope"?

Copy link
Author

Choose a reason for hiding this comment

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

Maybe this should be "global scope" for a global and "module scope" for a module? @DanielRosenwasser thoughts?

}
else {
return "unknown";
Expand Down
7 changes: 5 additions & 2 deletions tests/cases/fourslash/extract-method1.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,11 @@
//// }

goTo.select('start', 'end')
verify.refactorAvailable('Extract Method');
edit.applyRefactor('Extract Method', "scope_0");
edit.applyRefactor({
refactorName: "Extract Method",
actionName: "scope_0",
actionDescription: "Extract function into class 'Foo'",
});
verify.currentFileContentIs(
`class Foo {
someMethod(m: number) {
Expand Down
8 changes: 6 additions & 2 deletions tests/cases/fourslash/extract-method10.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
/// <reference path='fourslash.ts' />

//// (x => x)(/*1*/x => x/*2*/)(1);
//// (x => x)(/*1*/x => x/*2*/)(1);

goTo.select('1', '2');
edit.applyRefactor('Extract Method', 'scope_0');
edit.applyRefactor({
refactorName: "Extract Method",
actionName: 'scope_0',
actionDescription: "Extract function into this file",
});
12 changes: 10 additions & 2 deletions tests/cases/fourslash/extract-method13.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,18 @@
//// }

goTo.select('a', 'b');
edit.applyRefactor('Extract Method', 'scope_0');
edit.applyRefactor({
refactorName: "Extract Method",
actionName: "scope_0",
actionDescription: "Extract function into class 'C'",
});

goTo.select('c', 'd');
edit.applyRefactor('Extract Method', 'scope_0');
edit.applyRefactor({
refactorName: "Extract Method",
actionName: "scope_0",
actionDescription: "Extract function into class 'C'",
});

verify.currentFileContentIs(`class C {
static j = C.newFunction_1();
Expand Down
6 changes: 5 additions & 1 deletion tests/cases/fourslash/extract-method14.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,11 @@
//// }

goTo.select('a', 'b');
edit.applyRefactor('Extract Method', 'scope_1');
edit.applyRefactor({
refactorName: "Extract Method",
actionName: "scope_1",
actionDescription: "Extract function into this file",
});
verify.currentFileContentIs(`function foo() {
var i = 10;
var __return: any;
Expand Down
6 changes: 5 additions & 1 deletion tests/cases/fourslash/extract-method15.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,11 @@
//// }

goTo.select('a', 'b');
edit.applyRefactor('Extract Method', 'scope_1');
edit.applyRefactor({
refactorName: "Extract Method",
actionName: "scope_1",
actionDescription: "Extract function into this file",
});

verify.currentFileContentIs(`function foo() {
var i = 10;
Expand Down
7 changes: 5 additions & 2 deletions tests/cases/fourslash/extract-method18.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@
//// }

goTo.select('a', 'b')
verify.refactorAvailable('Extract Method');
edit.applyRefactor('Extract Method', "scope_1");
edit.applyRefactor({
refactorName: "Extract Method",
actionName: "scope_1",
actionDescription: "Extract function into this file",
});
verify.currentFileContentIs(`function fn() {
const x = { m: 1 };
newFunction(x);
Expand Down
9 changes: 6 additions & 3 deletions tests/cases/fourslash/extract-method19.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,15 @@
//// function fn() {
//// /*a*/console.log("hi");/*b*/
//// }
////
////
//// function newFunction() { }

goTo.select('a', 'b')
verify.refactorAvailable('Extract Method');
edit.applyRefactor('Extract Method', "scope_0");
edit.applyRefactor({
refactorName: "Extract Method",
actionName: "scope_0",
actionDescription: "Extract function into function 'fn'",
});
verify.currentFileContentIs(`function fn() {
newFunction_1();

Expand Down
7 changes: 5 additions & 2 deletions tests/cases/fourslash/extract-method2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,11 @@
//// }
//// }
goTo.select('start', 'end')
verify.refactorAvailable('Extract Method');
edit.applyRefactor('Extract Method', "scope_2");
edit.applyRefactor({
refactorName: "Extract Method",
actionName: "scope_2",
actionDescription: "Extract function into this file",
});
verify.currentFileContentIs(
`namespace NS {
class Q {
Expand Down
6 changes: 5 additions & 1 deletion tests/cases/fourslash/extract-method21.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,11 @@ goTo.select('start', 'end')

verify.refactorAvailable('Extract Method');

edit.applyRefactor('Extract Method', "scope_0");
edit.applyRefactor({
refactorName: "Extract Method",
actionName: "scope_0",
actionDescription: "Extract function into class 'Foo'",
});

verify.currentFileContentIs(`class Foo {
static method() {
Expand Down
6 changes: 5 additions & 1 deletion tests/cases/fourslash/extract-method24.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@
//// }

goTo.select('a', 'b')
edit.applyRefactor('Extract Method', 'scope_1');
edit.applyRefactor({
refactorName: "Extract Method",
actionName: "scope_1",
actionDescription: "Extract function into this file",
});
verify.currentFileContentIs(`function M() {
let a = [1,2,3];
let x = 0;
Expand Down
6 changes: 5 additions & 1 deletion tests/cases/fourslash/extract-method25.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@
//// }

goTo.select('a', 'b')
edit.applyRefactor('Extract Method', 'scope_0');
edit.applyRefactor({
refactorName: "Extract Method",
actionName: "scope_0",
actionDescription: "Extract function into function 'fn'",
});
verify.currentFileContentIs(`function fn() {
var q = newFunction()
q[0]++
Expand Down
2 changes: 1 addition & 1 deletion tests/cases/fourslash/extract-method3.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
//// }
//// }

// Don't offer to to 'extract method' a single identifier
// Don't offer to 'extract method' a single identifier

goTo.marker('a');
verify.not.refactorAvailable('Extract Method');
Expand Down
6 changes: 5 additions & 1 deletion tests/cases/fourslash/extract-method5.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,11 @@
//// }

goTo.select('start', 'end');
edit.applyRefactor('Extract Method', 'scope_0');
edit.applyRefactor({
refactorName: "Extract Method",
actionName: "scope_0",
actionDescription: "Extract function into function 'f'",
});
verify.currentFileContentIs(
`function f() {
var x: 1 | 2 | 3 = newFunction();
Expand Down
6 changes: 5 additions & 1 deletion tests/cases/fourslash/extract-method7.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@
//// }

goTo.select('a', 'b');
edit.applyRefactor('Extract Method', 'scope_0');
edit.applyRefactor({
refactorName: "Extract Method",
actionName: "scope_0",
actionDescription: "Extract function into this file",
});
verify.currentFileContentIs(`function fn(x = newFunction()) {
}
function newFunction() {
Expand Down
4 changes: 2 additions & 2 deletions tests/cases/fourslash/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ declare namespace FourSlashInterface {
codeFixDiagnosticsAvailableAtMarkers(markerNames: string[], diagnosticCode?: number): void;
applicableRefactorAvailableForRange(): void;

refactorAvailable(name?: string, subName?: string);
refactorAvailable(name: string, actionName?: string);
}
class verify extends verifyNegatable {
assertHasRanges(ranges: Range[]): void;
Expand Down Expand Up @@ -310,7 +310,7 @@ declare namespace FourSlashInterface {
enableFormatting(): void;
disableFormatting(): void;

applyRefactor(refactorName: string, actionName: string): void;
applyRefactor(options: { refactorName: string, actionName: string, actionDescription: string }): void;
}
class debug {
printCurrentParameterHelp(): void;
Expand Down