| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
Anyone want to take a look? The VS side is about to merge |
Sorry, something went wrong.
| Warning, | ||
| Error, | ||
| Message, | ||
| CodeFix |
There was a problem hiding this comment.
i would call this Suggestion.
Sorry, something went wrong.
There was a problem hiding this comment.
Removed
Sorry, something went wrong.
| export type GetApplicableRefactorsRequestArgs = FileLocationOrRangeRequestArgs; | ||
|
|
||
| export interface ApplicableRefactorInfo { | ||
| refactorName: string; |
There was a problem hiding this comment.
just name
Sorry, something went wrong.
| /* @internal */ | ||
| export type GetCodeFixesFull = "getCodeFixes-full"; | ||
| export type GetSupportedCodeFixes = "getSupportedCodeFixes"; | ||
| export type GetCodeFixDiagnostics = "getCodeFixDiagnostics"; |
There was a problem hiding this comment.
getSuggestionDiagnostics
Sorry, something went wrong.
There was a problem hiding this comment.
Removed
Sorry, something went wrong.
| }); | ||
|
|
||
| function createCodeFixDiagnosticIfApplicable(node: Node, context: CodeFixDiagnoseContext): Diagnostic | undefined { | ||
| if (!isSourceFileJavaScript(context.boundSourceFile)) { |
There was a problem hiding this comment.
this is not going to scale as we add code fixes. we can not call every code fix on every node on the tree, we need a more efficient way.
We could make codeFix register a SyntaxKind it is interested in, this will save us the dispatch on every node at least.
Alternatively, we can have the code fix return a Boolean whether to keep walking or not, this way we avoid walking expressions that we do not care about.
Sorry, something went wrong.
There was a problem hiding this comment.
Codefix feature removed
Sorry, something went wrong.
| } | ||
|
|
||
| export function getCodeFixDiagnosticsForNode(context: CodeFixDiagnoseContext, node: Node): Diagnostic[] | undefined { | ||
| let result: Diagnostic[]; |
There was a problem hiding this comment.
i think we need to optimize this function and getCodeFixDiagnostics.
Sorry, something went wrong.
|
|
||
| const checker = context.program.getTypeChecker(); | ||
| const symbol = checker.getSymbolAtLocation(node); | ||
| if (isClassLikeSymbol(symbol)) { |
There was a problem hiding this comment.
i would say this should be limited to the declaration, not on every reference.
I would also say you do not need the checker here.
Sorry, something went wrong.
| export interface CodeFix { | ||
| errorCodes: number[]; | ||
| getCodeActions(context: CodeFixContext): CodeAction[] | undefined; | ||
| createCodeFixDiagnosticIfApplicable?(node: Node, context: CodeFixDiagnoseContext): Diagnostic | undefined; |
There was a problem hiding this comment.
Ideally we want this to be syntactic in nature, passing in the program allows for getting the checker. this can be expensive if the codeFix author do not know what they are doing, consider not passing in the program, but just a sourceFile and options.
Sorry, something went wrong.
There was a problem hiding this comment.
We also want to use the same mechanism for refactorings. a refactoring should have isApplicable, and getCodeActions. I would recommend switching this into isApplicable, and an optional diagnostic message.
Sorry, something went wrong.
| getCodeActions(context: RefactorContext, positionOrRange: number | TextRange): CodeAction[]; | ||
|
|
||
| /** A fast syntactic check to see if the refactor is applicable at given position. */ | ||
| isApplicableForPositionOrRange(context: LightRefactorContext, positionOrRange: number | TextRange): boolean; |
There was a problem hiding this comment.
consider unifying this with the codeFix isApplicable. ideally we can have one implementation that serves as both a refactor and a codefix + suggestion provider.
Sorry, something went wrong.
|
|
||
| const asyncSuffixRefactor: Refactor = { | ||
| name: "Add Async suffix", | ||
| description: "Add an 'Async' suffix to async function declarations", |
There was a problem hiding this comment.
should be a diagnostics message for localizability.
Sorry, something went wrong.
| const token = getTokenAtPosition(nonBoundSourceFile, tokenPos); | ||
|
|
||
| let node = token; | ||
| while (node) { |
There was a problem hiding this comment.
It is kind of random that on any expression i can refactor the containing function to include async. i would say this has to be on function/method declaration at least.
Sorry, something went wrong.
| getCodeActions(context: RefactorContext, positionOrRange: number | TextRange): CodeAction[]; | ||
|
|
||
| /** A fast syntactic check to see if the refactor is applicable at given position. */ | ||
| isApplicableForPositionOrRange(context: QueryRefactorContext, positionOrRange: number | TextRange): boolean; |
There was a problem hiding this comment.
just isApplicable, the positionOrRange is implied from the arguments.
Sorry, something went wrong.
| /* @internal */ | ||
| namespace ts.refactor { | ||
| const asyncSuffixRefactor: Refactor = { | ||
| name: "Add Async suffix", |
There was a problem hiding this comment.
This should be in diagnostics
Sorry, something went wrong.
There was a problem hiding this comment.
Removed this refactor
Sorry, something went wrong.
| namespace ts.refactor { | ||
| const asyncSuffixRefactor: Refactor = { | ||
| name: "Add Async suffix", | ||
| description: "Add an 'Async' suffix to async function declarations", |
There was a problem hiding this comment.
should be in diagnostics to ensure localizatability.
Sorry, something went wrong.
| }]; | ||
| } | ||
|
|
||
| function isApplicableForPositionOrRange(context: RefactorContext, positionOrRange: number | TextRange): boolean { |
There was a problem hiding this comment.
kinda silly comment, but can we have (startPosition, endPoisition) instead of positionOrRange? every refactor author has to write the same check typeof positionOrRange === "number"?...
Sorry, something went wrong.
There was a problem hiding this comment.
Actually why not just put these on the RefactorContext? we put the errorCode on the codeFix context.
Sorry, something went wrong.
There was a problem hiding this comment.
Makes sense
Sorry, something went wrong.
|
|
||
| let node = token; | ||
| while (node) { | ||
| if (node.kind === SyntaxKind.FunctionDeclaration) { |
There was a problem hiding this comment.
i would use isNameOfFunctionDeclaration or something similar.
Sorry, something went wrong.
There was a problem hiding this comment.
i would check if token is one of identifier, function keyword, public, private, protected, or static , and their parent is a FunctionDeclatation or MethodDeclaration.
Sorry, something went wrong.
There was a problem hiding this comment.
File removed
Sorry, something went wrong.
| } | ||
|
|
||
| // all static members are stored in the "exports" array of symbol | ||
| if (symbol.exports) { |
There was a problem hiding this comment.
Is this intended for a .ts or a .js file?
I think it is a .js file, since the binding for the special properties do not work in .ts files any way.
If my understanding is correct, then for static members we can not just add declarations for them, this will result in invalid .js class.
this also applies to the property declarations and assignments..
If we want this to work in a .ts file, then we need to do something with the inference to make it work first.
I would also recommend adding a check at the top of isApplicable to return if !inJavaScriptFile(node)
Sorry, something went wrong.
There was a problem hiding this comment.
also for member initialization, I wounder if we can even do these correctellly...
var c = function() { this.list.push(1); }
c.prototype.list = [];is not the same as:
class c {
constructor() { this.list.push(1); }
list = [];let a side that it is not valid JS strictly speaking.
Sorry, something went wrong.
There was a problem hiding this comment.
Fixed these up. Non-func-expr things remain as assignments following the class body
Sorry, something went wrong.
| positionOrRange: number | TextRange, | ||
| refactorName: string): CodeAction[] | undefined { | ||
|
|
||
| const context: RefactorContext = { |
There was a problem hiding this comment.
consider in-lining the const in the call and using the contextual type instead.
Sorry, something went wrong.
| positionOrRange: number | TextRange): ApplicableRefactorInfo[] | undefined { | ||
|
|
||
| let results: ApplicableRefactorInfo[]; | ||
| refactors.forEach(refactor => { |
There was a problem hiding this comment.
I would check the cancellation token here.
Sorry, something went wrong.
| program: Program; | ||
| } | ||
|
|
||
| export interface RefactorContext extends QueryRefactorContext { |
There was a problem hiding this comment.
what is the value of the split here?
Sorry, something went wrong.
|
|
||
| export interface RefactorContext extends QueryRefactorContext { | ||
| newLineCharacter: string; | ||
| rulesProvider: formatting.RulesProvider; |
There was a problem hiding this comment.
what about cancellation token?
Sorry, something went wrong.
There was a problem hiding this comment.
consider exposing CodeFixContext minus errorCode.
Sorry, something went wrong.
|
|
||
| export type GetApplicableRefactors = "getApplicableRefactors"; | ||
| export type GetRefactorCodeActions = "getRefactorCodeActions"; | ||
| export type GetRefactorCodeActionsFull = "getRefactorCodeActions-full"; |
There was a problem hiding this comment.
This should be marked as /* @internal */
Sorry, something went wrong.
|
|
||
| export type RefactorCodeActions = { | ||
| actions: protocol.CodeAction[]; | ||
| renameLocation?: number |
There was a problem hiding this comment.
A comment here would be good.
Sorry, something went wrong.
| program: Program; | ||
| newLineCharacter: string; | ||
| rulesProvider?: formatting.RulesProvider; | ||
| cancellationToken?: CancellationToken; |
There was a problem hiding this comment.
why is this optional?
Sorry, something went wrong.
| endPosition?: number; | ||
| program: Program; | ||
| newLineCharacter: string; | ||
| rulesProvider?: formatting.RulesProvider; |
There was a problem hiding this comment.
why is this optional?
Sorry, something went wrong.
Fixes microsoft#25739, from microsoft/TypeScript#15569 Prototype of refactoring support for ts 2.4
* Prototype TS/JS Refactoring Provider Fixes #25739, from microsoft/TypeScript#15569 Prototype of refactoring support for ts 2.4 * Adding error reporting * Updating for new API * show quick pick for non-inlinable refactrings
| Back | FazBrowse Home | New Git URL |
Picks up where #14624 left off.