extractMethod: Support renameLocation · Pull Request #18050 · microsoft/TypeScript

ghost · 2017-08-25T21:01:29Z

Fixes #17852
Does not fix #18048
EDIT: Now fixes both

mhegazy · 2017-08-25T22:03:16Z

src/services/refactors/extractMethod.ts

+        const { scopes, readsAndWrites: { errorsPerScope } } = getPossibleExtractionsWorker(targetRange, context);
+        // Need the inner type annotation to avoid https://github.com/Microsoft/TypeScript/issues/7547
+        return scopes.map((scope, i): PossibleExtraction => {
+            const errors = errorsPerScope[i];


can be inlined

ghost · 2017-09-08T20:55:06Z

@mhegazy @amcasey Could you review again?

mhegazy · 2017-09-08T21:38:22Z

src/services/refactors/extractMethod.ts

+            Debug.assert(fileName === renameFilename);
+            for (const change of textChanges) {
+                const { span, newText } = change;
+                const index = newText.indexOf(functionNameText);


we do not know if the name is really unique text wise. we only look if it was declared as an identifier, but not in a comment or string literal for instance.

this also relies on the use site being inserted in the file before the definition. we should add a comment to that meaning in case we change where the function declaration is added.

It would definitely be preferable not to assume that the new declaration is inserted after the extracted range because that will almost certainly not be the case when we start extracting locals.

amcasey · 2017-09-08T21:51:48Z

src/harness/fourslash.ts

+                }
+            }
+            else {
+                // TODO: test editInfo.renameFilename too


Maybe just assert that it's non-empty for now?

amcasey · 2017-09-08T21:53:07Z

src/harness/unittests/extractMethods.ts

                for (const r of results) {
-                    const changes = refactor.extractMethod.getPossibleExtractions(result.targetRange, context, results.indexOf(r))[0].changes;
+                    const { renameLocation, edits } = refactor.extractMethod.getPossibleExtractionAtIndex(result.targetRange, context, results.indexOf(r));
+                    Debug.assert(edits.length === 1);


use assert.equal?

mhegazy · 2017-09-08T22:19:29Z

Please port this to release-2.5

amcasey · 2017-09-08T22:25:15Z

src/services/refactors/extractMethod.ts

    }

+    // exported only for tests
+    export function getPossibleExtractionAtIndex(targetRange: TargetRange, context: RefactorContext, requestedChangesIndex: number): RefactorEditInfo {


Just "getExtractionAtIndex"?

amcasey · 2017-09-08T22:29:10Z

src/services/refactors/extractMethod.ts

    }

-    function getUniqueName(isNameOkay: (name: string) => boolean) {
+    function getUniqueName(fileText: string): string {


So, somewhat ironically, if you were to add a comment //TODO newFunction_1 goes here, we would call the new function "newFunction_2"?

Would it be easier to use something temporary, like a GUID, until the replacement has been accomplished and then swap it for the name we would originally have generated?

That might be doable, but we might (might not) need the GUID to have an identical length. But you could still calculate them together inside getUniqueName.

amcasey · 2017-09-08T22:30:52Z

src/services/refactors/extractMethod.ts

-            isClassLike(scope) ? createPropertyAccess(range.facts & RangeFacts.InStaticRegion ? createIdentifier(scope.name.getText()) : createThis(), functionReference) : functionReference,
+        const called = getCalledExpression(scope, range, functionNameText);
+
+        let call: Expression = createCall(called,


Missing linebreak before argument?

amcasey · 2017-09-08T22:40:28Z

src/services/refactors/extractMethod.ts

+            Debug.assert(fileName === renameFilename);
+            for (const change of textChanges) {
+                const { span, newText } = change;
+                // Note: We are assuming that the call expression comes before the function declaration,


Can you please add TODO (acasey) (or TODO (17098))? This assumption will be incorrect as soon as we implement Extract Local.

amcasey · 2017-09-08T22:41:51Z

src/services/refactors/extractMethod.ts

-        usages: Map<UsageEntry>;
-        typeParameterUsages: Map<TypeParameter>; // Key is type ID
-        substitutions: Map<Node>;
+        readonly usages: Map<UsageEntry>;


What happens when a property of an exported type depends on a non-exported type?

Users can't reference the type itself, but they can get a variable of the type. Pretty annoying. But it looks like this doesn't actually need to be exported.

amcasey · 2017-09-08T22:44:20Z

tests/cases/fourslash/extract-method14.ts

    var i = 10;
    var __return: any;
-    ({ __return, i } = newFunction(i));
+    ({ __return, i } = n/*RENAME*/ewFunction(i));


Why is the location within the identifier?

Added #18349

amcasey

Our method of coming up with a unique name seems slightly crazy to me (esp for large files), but this generally makes sense.

extractMethod: Support renameLocation

b899d10

ghost requested a review from RyanCavanaugh August 25, 2017 21:01

msftclas added the cla-not-required label Aug 25, 2017

ghost requested a review from amcasey August 25, 2017 21:01

Add tslint disable

36d59af

mhegazy reviewed Aug 25, 2017

View reviewed changes

Andy Hanson added 5 commits September 5, 2017 10:44

Merge branch 'master' into extract_rename

fd1a0af

Properly analyze list of changes to always get a correct rename location

0aa0ebf

Merge branch 'master' into extract_rename

fbc6484

Merge branch 'master' into extract_rename

ff96da7

Update test

f592be8

mhegazy reviewed Sep 8, 2017

View reviewed changes

amcasey reviewed Sep 8, 2017

View reviewed changes

Andy Hanson added 2 commits September 8, 2017 15:01

Ensure name is really unique

a150596

Improvements to test code

6953143

mhegazy approved these changes Sep 8, 2017

View reviewed changes

amcasey reviewed Sep 8, 2017

View reviewed changes

amcasey approved these changes Sep 8, 2017

View reviewed changes

ghost mentioned this pull request Sep 8, 2017

Apparent off-by-one error for extract method rename #18349

Closed

Respond to PR comments

6303626

ghost mentioned this pull request Sep 8, 2017

extractMethod: Support renameLocation #18351

Merged

Merge branch 'master' into extract_rename

2ab5894

ghost merged commit c3199c7 into master Sep 13, 2017

ghost deleted the extract_rename branch September 13, 2017 16:02

microsoft locked and limited conversation to collaborators Jun 14, 2018

This pull request was closed.

Conversation

ghost commented Aug 25, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhegazy commented Sep 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amcasey left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ghost commented Aug 25, 2017 •

edited by ghost

Loading