Making Move To File Action appear less often

[aiday-mar]

Fixes #56416

This work is done in relation to conversation with @navya9singh. From the conversation, the following PR aims to do the following:

Add a condition in getAvailableActions() to decide when you want the refactor to not show up. The strategy to follow for now is to not offer the refactor if both ends of the initial selection range are inside a BlockLike node type (other than the source file).

[typescript-bot]

Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page.

Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up.

[typescript-bot]

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

[DanielRosenwasser]

We generally expect a test to exhibit new behavior. Language service tests should be placed in tests/cases/fourslash/ and you can take a look at existing refactoring tests.

[aiday-mar]

Hi thank you for the review comments. I have been working on the changes, and was looking at adding a test. More specifically I talked to one of your colleagues about how tests work in the TypeScript repo and this person recommended to add a test of the following type:

/// <reference path='fourslash.ts' />

////function f3(x, /*e*//*f*/{ y }) {
////    return { x, y: 1 };
////}

goTo.select("e", "f");
verify.not.refactorAvailable("Move to file");

I was debugging this test and I noticed that due to the current structure of the code, the move to refactoring is never made available in a testing context. More specifically by tracking the method refactorAvailable, it leads to verifyRefactorAvailable, which ultimately leads to getApplicableRefactors where the boolean includeInteractiveActions is undefined. Now because this boolean is undefined, later the method getAvailableActions returns an empty array through an early return statement independently of the block conditions later. In this manner, I believe, this test may perhaps not be testing the behavior introduced in this PR. I was therefore wondering if you believe this test is good, and if not, which method I could use to write the new test? Thank you.

[navya9singh]

Hi thank you for the review comments. I have been working on the changes, and was looking at adding a test. More specifically I talked to one of your colleagues about how tests work in the TypeScript repo and this person recommended to add a test of the following type:

/// <reference path='fourslash.ts' />

////function f3(x, /*e*//*f*/{ y }) {
////    return { x, y: 1 };
////}

goTo.select("e", "f");
verify.not.refactorAvailable("Move to file");

I was debugging this test and I noticed that due to the current structure of the code, the move to refactoring is never made available in a testing context. More specifically by tracking the method refactorAvailable, it leads to verifyRefactorAvailable, which ultimately leads to getApplicableRefactors where the boolean includeInteractiveActions is undefined. Now because this boolean is undefined, later the method getAvailableActions returns an empty array through an early return statement independently of the block conditions later. In this manner, I believe, this test may perhaps not be testing the behavior introduced in this PR. I was therefore wondering if you believe this test is good, and if not, which method I could use to write the new test? Thank you.

You're right about the interactiveActions always being undefined in this test case and so getAvailableActions always returning an empty array. Looking at this I think that we need to add a function called moveToFileVerifyAction or something similar under verifyNegatable class in fourslash.ts to actually be able to test if this action should be available or not. Does that sound okay @andrewbranch

[andrewbranch]

Is there any reason not to have verifyRefactorAvailable always pass includeInteractiveActions?

[aiday-mar]

So if that suits everyone, could we just set the boolean includeInteractiveActions to true?

[aiday-mar]

Seeing as there is a thumbs up on my last message I will make the boolean true. I am trying to construct a test that will make the condition startNode.kind !== SyntaxKind.SourceFile && isBlockLike(startNode) true. I have written the following test, which I though would make the condition evaluate to true, since the selection start is inside of a module keyword.

/// <reference path='fourslash.ts' />

////modu/*e*/le My.App {
////    export var appModule = angular.module("app", [
////    ]).config([() => {
////        configureStates/*1*/($stateProvider);
////    }]).run(My.App.setup);
////}/*f*/

goTo.select("e", "f");
verify.not.refactorAvailable("Move to file");

Upon testing however the token at the start of the selection was evaluated to be kind ModuleKeyword not ModuleBlock or Block. I tried several other examples, aiming to make the isBlockLike method return true. I was wondering if you have an example of a test with a given selection where the selection start node would be evaluated to be a Block node?

As a small note, we would like to merge this PR this week if possible, as it is part of the VS Code iteration plan for the month of February. In light of this, I was wondering if we could schedule a meeting this week to discuss the PR in more detail

[navya9singh]

I looked at extractSymbol for some reference and one simple example for this case would be :

 namespace ns {
    /*a*/export function fn() {

    }
    fn();
    /*b*/
}

In here, I believe you will have to change startNode.kind !== SyntaxKind.SourceFile && isBlockLike(startNode) condition to instead be a loop that starts at the parent of the startNode and traverses up the tree to check if the corresponding node is a blockLike node. Same will have to be done for the endNode, and if both conditions are true, then Move to file should not show up.

[aiday-mar]

So I implemented the idea you mentioned in your last message. I created two tests, one with the text you suggested, where no move to action should be returned, and one where the move to action should be returned. While writing these tests I noticed a similar problem to the one we had with the boolean includeInteractiveActions . The context.preferences object is always empty in a testing context, because it is not surfaced in the refactorAvailable method as a modifiable parameter. I therefore surfaced both the preferences and the includeInteractiveActions options in the refactorAvailable method. With this addition, this should provide sufficient coverage for the code change proposed in this PR. Could I ask for another review of the code? Let me know if there is anything I can change to improve the PR.

[aiday-mar]

Hi @navya9singh thank you for the review comments. I made the changes requested.

[aiday-mar]

Hi @andrewbranch @navya9singh if the code looks good, would it be possible to receive an approval for merging the code? On the VS Code side, we would like to get this PR merged by the end of this week.

[andrewbranch]

Looks good overall, just a few nits. Thanks!

[aiday-mar]

Hi thank you for all your review comments, I have made the latest changes requested.

[aiday-mar]

Hi @DanielRosenwasser to merge this PR, it appears that I may need your approval on the changes you requested. I have made the changes you mentioned.

[DanielRosenwasser]

@mu578 I've never seen you on this repository, but here you must adhere to the code of conduct. You must keep feedback respectful and objective.