GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Codefix for implementing interfaces by paulvanbrenk · Pull Request #11547 · microsoft/TypeScript Conversation
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (token.kind === SyntaxKind.Identifier && token.parent.parent.kind === SyntaxKind.HeritageClause) { | ||
| const children = (<HeritageClause>token.parent.parent).getChildren(); | ||
| ts.forEach(children, child => { | ||
| if (child.kind === SyntaxKind.ExtendsKeyword) { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
this is not sufficient. what if the class already has an implements clause. you want to first look for implements clause, and if you found one, move the interface to the implements list otherwise rename the extends to implements. one option is to generate two edits, one that always removes the extends clause all together, and another one that adds the target to the right place.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Also please add tests.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Fixed. The edit is to change extends -> implements, and if implements is already present, change implements -> ','
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| @@ -0,0 +1,268 @@ | |||
| /* @internal */ | |||
| namespace ts.codefix { | |||
| registerCodeFix({ | |||
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
please split these into different files. let's keep each code fix in its own file.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
you can still share the helpers between files if you need to.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Split, and helpers moved to src/services/utilities.ts
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| let textChanges: TextChange[] = []; | ||
|
|
||
| if (token.kind === SyntaxKind.Identifier && token.parent.kind === SyntaxKind.ClassDeclaration) { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
what about classExpression? var x = class C implements I { }? use isClassLike instead
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Changed.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| let textChanges: TextChange[] = []; | ||
|
|
||
| if (token.kind === SyntaxKind.Identifier && token.parent.kind === SyntaxKind.ClassDeclaration) { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
use isClassLike
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Changed
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| const token = getTokenAtPosition(sourceFile, start); | ||
| const checker = context.program.getTypeChecker(); | ||
|
|
||
| let textChanges: TextChange[] = []; |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
what is the point of this initialization, you can just override it below. consider just leaving this as undefined, and checking for undefined below.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Refactored
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| if (token.kind === SyntaxKind.Identifier && token.parent.kind === SyntaxKind.ClassDeclaration) { | ||
| const classDeclaration = <ClassDeclaration>token.parent; | ||
| const startPos = classDeclaration.members.pos; | ||
| const classMembers = getClassMembers(classDeclaration); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
you just need the abstract ones.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Changed.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| const classMembers = getClassMembers(classDeclaration); | ||
| const trackingAddedMembers: string[] = []; | ||
| const extendsClause = ts.getClassExtendsHeritageClauseElement(classDeclaration); | ||
| textChanges = textChanges.concat(getChanges(extendsClause, classMembers, startPos, checker, /*reference*/ false, trackingAddedMembers, context.newLineCharacter)); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This does not handle properties declared in the constructor as parameter property declarations, e.g. this should only add b and not a.
interface I { a: number, b:string; } class C implements I { constructor(public a) {} }
Please add tests.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| if (interfaceMembers[j].name && existingMembers.indexOf(interfaceMembers[j].name.getText()) === -1) { | ||
| if (interfaceMembers[j].kind === SyntaxKind.PropertySignature) { | ||
| const interfaceProperty = <PropertySignature>interfaceMembers[j]; | ||
| if (trackingAddedMembers.indexOf(interfaceProperty.name.getText()) === -1) { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
this does not handle computed properties correctly. e.g. [Symbol.Iterator]. please add tests.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| } | ||
|
|
||
| function getMembersAndStartPosFromReference(variableDeclaration: VariableDeclaration): { startPos: number, members: string[] } { | ||
| const children = variableDeclaration.getChildren(); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
variableDeclaration.initializer ?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| /* @internal */ | ||
| namespace ts.codefix { | ||
| registerCodeFix({ | ||
| errorCodes: [Diagnostics.Type_0_is_not_assignable_to_type_1.code], |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
this code fix is not nearly close to being complete. please remove this from this patch and added by itself later on. i am not sure we should do this either. there are issues that are not fixable. and we are not detecting them and handling them correctly.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Removed, and tests changed.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| } | ||
| `); | ||
| verify.not.codeFixAvailable(); | ||
| // verify.codeFixAtPosition(` |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
why are these still here?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Removed.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Still need to add localization strings
| getReturnTypeOfSignature, | ||
| getNonNullableType, | ||
| getSymbolsInScope, | ||
| createSymbol, |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
i would say use new Symbol() here.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Changed.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| getIndexTypeOfType, | ||
| getBaseTypes, | ||
| getUnionType, | ||
| getIntersectionType, |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
if this is not used i would remove it for now.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Removed.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| getExportSpecifierLocalTargetSymbol, | ||
| getTypeAtLocation: getTypeOfNode, | ||
| getPropertySymbolOfDestructuringAssignment, | ||
| signatureToString, |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
consider inlining these two using checker.getSymbolDisplayBuilder().
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
signatureToString is needed independently in the checker anyways, so I'd prefer to avoid copying the code if it can be avoided.
indexSignatureToString: Only used in one place in the current PR, but we may find it useful in a future codefix for adding index signatures. Will inline for now, but may have to re-declare it in the future.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| const classDecl = <ClassDeclaration>token.parent; | ||
| const startPos = classDecl.members.pos; | ||
|
|
||
| const InstantiatedExtendsType = checker.getTypeFromTypeReference(getClassExtendsHeritageClauseElement(classDecl)) as InterfaceType; |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
please add a test for expression with type arguments case:
function foo<T>(a: T) { abstract class C<U> { a: T; b: U; abstract method2(); } return C; } class B extends foo("s")<number> { method() { } }
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Added.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| const decls = symbol.getDeclarations(); | ||
| Debug.assert(!!(decls && decls.length > 0)); | ||
| const flags = getModifierFlags(decls[0]); | ||
| return !(flags & ModifierFlags.Private) && !!(flags & ModifierFlags.Abstract); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
i would ignore private here.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
return symbol.valueDeclaration.flags & ModifierFlags.Private
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Needed to suppress fix when the user creates a class with a private abstract property/member. Is there a better way to detect general errors on a declaration/node? This would be useful to suppress codefixes in many situations.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| } | ||
|
|
||
| function symbolRefersToNonPrivateMember(symbol: Symbol): boolean { | ||
| const decls = symbol.getDeclarations(); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
just:
return symbol.valueDecaration.flags & ModifierFlags.Private
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
EDIT: moved response to the right place.
Made the change here.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| function createParameterDeclaration(index: number, minArgCount: number, typeNode: TypeNode, enclosingSignatureDeclaration: SignatureDeclaration): ParameterDeclaration { | ||
| const newParameter = createNode(SyntaxKind.Parameter) as ParameterDeclaration; | ||
| newParameter.symbol = checker.createSymbol(SymbolFlags.FunctionScopedVariable, "arg" + index); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
i would rather we use a name form one of hte signatures.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| } | ||
|
|
||
| if (hasRestParameter) { | ||
| const anyArrayTypeNode = createNode(SyntaxKind.ArrayType) as ArrayTypeNode; |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
consider making these implicitAny's instead by not setting a type, and handling this in the writer instead.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Changed.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Most of my reviews are updated
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.