-
Notifications
You must be signed in to change notification settings - Fork 13.2k
Pvb/codeaction/api #10185
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
Pvb/codeaction/api #10185
Conversation
src/compiler/core.ts
Outdated
| return -1; | ||
| } | ||
|
|
||
| export function firstOrUndefined<T>(array: T[], predicate: (x: T) => boolean): T { |
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.
forEach
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.
forEach is slightly different. Also this is usually called find.
| const fixes = codeFixes[context.errorCode]; | ||
| let allActions: CodeAction[] = []; | ||
|
|
||
| Debug.assert(fixes && fixes.length > 0, "No fixes found for error: '${errorCode}'."); |
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.
isn't it too strict?
| const superCall = findSuperCall((<ConstructorDeclaration>constructor).body); | ||
| Debug.assert(!!superCall, "Failed to find super call."); | ||
|
|
||
| const newPosition = getOpenBraceEnd(<ConstructorDeclaration>constructor, sourceFile); |
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.
can you add a test for the case where this is an argument to super
super(this.x);|
Please add this to the tsserver, you will need to define a new request and response kinds in server\protocol.d.ts, and add a handler for it in server\session.ts |
src/services/codefixes/references.ts
Outdated
| @@ -0,0 +1,3 @@ | |||
| ///<reference path='..\services.ts' /> | |||
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.
we stoped using references for a while. they should be picked up by the tsconfig.json in this folder any ways. so please remove this.
# Conflicts: # src/compiler/diagnosticMessages.json # src/services/services.ts
…eScript into pvb/codeaction/api
# Conflicts: # src/services/types.ts
| position?: number; | ||
| } | ||
|
|
||
| /** |
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.
nit Code Fixes instead of codefixed
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.
done
src/server/protocol.d.ts
Outdated
| textChanges: CodeEdit[]; | ||
| } | ||
|
|
||
| export interface CodeActionResponse extends Response { |
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.
CodeFixResponse
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.
done
| /** Description of the code action to display in the UI of the editor */ | ||
| description: string; | ||
| /** Text changes to apply to each file as part of the code action */ | ||
| changes: FileCodeEdits[]; |
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.
CodeAction [] ?
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.
yep
src/server/protocol.d.ts
Outdated
| /** | ||
| * Changes to apply to each file as part of the code action. | ||
| */ | ||
| changes: FileTextChanges[]; |
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.
CodeEdit [] instead.
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.
see formatting for example.
src/server/protocol.d.ts
Outdated
| textChanges: TextChange[]; | ||
| } | ||
|
|
||
| export class TextChange { |
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.
not needed.
src/server/session.ts
Outdated
| description: source.description, | ||
| changes: source.changes.map(change => ({ | ||
| fileName: change.fileName, | ||
| textChanges: change.textChanges.map(textChange => ({ |
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.
extract the common part between here and getFormattingEditsForRange in convertTextChangeToCodeEdit
src/server/client.ts
Outdated
| } | ||
|
|
||
| getCodeFixesAtPosition(fileName: string, start: number, end: number, errorCodes: number[]): ts.CodeAction[] { | ||
| throw new Error("Not Implemented Yet."); |
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.
why? add implementation here, and add a test in tests\cases\fourslash\server
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.
done
src/services/shims.ts
Outdated
| */ | ||
| isValidBraceCompletionAtPosition(fileName: string, position: number, openingBrace: number): string; | ||
|
|
||
| getCodeFixesAtPosition(fileName: string, start: number, end: number, errorCodes: string): string; |
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.
we do not need to update the shims, since we moved to tsserver
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.
removed
| newLineCharacter: newLineChar | ||
| }; | ||
|
|
||
| const fixes = codefix.getFixes(context); |
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.
check the cancellation token for every iteration.
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.
done
| namespace ts { | ||
| export interface CodeFix { | ||
| errorCodes: number[]; | ||
| getCodeActions(context: CodeFixContext): CodeAction[]; |
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.
getCodeActions(context: CodeFixContext): CodeAction[] | undefined to allow strictNullChecks to work for users implementing it
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.
done
src/server/protocol.d.ts
Outdated
| body?: NavigationBarItem[]; | ||
| } | ||
|
|
||
| export interface CodeAction { |
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.
this is already defined above.
| export const CompilerOptionsForInferredProjects = "compilerOptionsForInferredProjects"; | ||
| export const GetCodeFixes = "getCodeFixes"; | ||
| export const GetCodeFixesFull = "getCodeFixes-full"; | ||
| export const GetSupportedCodeFixes = "getSupportedCodeFixes"; |
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.
i do not see the definition of the request for this command in protocol.d.ts
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.
can you also add a test for it.
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.
There are no arguments for the command so no need for a request
| }); | ||
| } | ||
|
|
||
| export function getSupportedErrorCodes() { |
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.
Any reason this returns a string[] and not a number[]? Error codes are always numbers, so you could map this with Number.parseInt.
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.
Yes, while we currently only return numbers for errorcodes, in the future e.g. a linter extension could prepend the errorcodes they return with a string. That way they can be differentiated and handled by separate 'fixers', even when they have the same error number.
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.
Maybe the definitions of errorCode and errorCodes above (in CodeFixContent and CodeFix) should use string then?
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.
You make a good point, I'll update that.
Add CodeAction API