-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Conversation
update @aiday-mar, looks like private toTsTriggerReason(context: vscode.CodeActionContext): Proto.RefactorTriggerReason | undefined {
return context.triggerKind === vscode.CodeActionTriggerKind.Invoke ? 'invoked' : 'implicit';
} which gets propagated thru |
Looks good, thanks! |
cc. @andrewbranch @navya9singh @DanielRosenwasser (realized i never pinged or mentioned you guys on this 😨) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@andrewbranch @navya9singh awesome thanks guys! feel free to merge when ready (I'm not authorized here 🔢 ) |
Fixes ##58387
as discussed in the sync, we added the additional check to
implicit
and also mirrored the fixes frommove to file
formove 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 thatmove to new file
will likely be deprecated anyways eventually.regarding changing:
verify.moveToNewFile({
TypeScript/tests/cases/fourslash/moveToNewFile_prologueDirectives4.ts
Line 8 in 5c21b7f
since
getApplicableRefactors
defaults toimplicit
:TypeScript/src/harness/fourslashImpl.ts
Line 4454 in 5c21b7f
the test was changed to have no refactorings (to match our proposed case).
regarding
implicit
vsinvoked
:Check for
implicit
to follow our idea formanual 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