Skip to content
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

move to code actions update and tests #58548

Merged
merged 3 commits into from
May 23, 2024

Conversation

justschen
Copy link
Contributor

Fixes ##58387

as discussed in the sync, we added the additional check to implicit and also mirrored the fixes from move to file for move to new file.

a lot of tests were added (if excessive we can delete), and fixes to move to new file were added but also understand that move to new file will likely be deprecated anyways eventually.


regarding changing:

since getApplicableRefactors defaults to implicit:

private getApplicableRefactors(rangeOrMarker: Range | Marker, preferences = ts.emptyOptions, triggerReason: ts.RefactorTriggerReason = "implicit", kind?: string, includeInteractiveActions?: boolean): readonly ts.ApplicableRefactorInfo[] {

the test was changed to have no refactorings (to match our proposed case).


regarding implicit vs invoked:

Check for implicit to follow our idea for manual requested in vs code vs. not manually requested in vs code, but let me know if our logic here makes sense!

cc. @aiday-mar @mjbvz

@justschen justschen changed the title Justin/move to move to code actions update and tests May 16, 2024
@justschen
Copy link
Contributor Author

update @aiday-mar, looks like triggerReason gets passed all the way from typescript extension:

https://github.com/microsoft/vscode/blob/cdf94536b9dafc36582d2ecfd246897c6feb7d72/extensions/typescript-language-features/src/languageFeatures/refactor.ts#L551

private toTsTriggerReason(context: vscode.CodeActionContext): Proto.RefactorTriggerReason | undefined {
	return context.triggerKind === vscode.CodeActionTriggerKind.Invoke ? 'invoked' : 'implicit';
}

which gets propagated thru CodeActionTriggerKind between automatic and invoke which we set in the model 👍

@aiday-mar
Copy link
Contributor

Looks good, thanks!

@justschen
Copy link
Contributor Author

cc. @andrewbranch @navya9singh @DanielRosenwasser (realized i never pinged or mentioned you guys on this 😨)

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

Thanks!

@justschen
Copy link
Contributor Author

@andrewbranch @navya9singh awesome thanks guys! feel free to merge when ready (I'm not authorized here 🔢 )

@andrewbranch andrewbranch merged commit abc37af into microsoft:main May 23, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants