add quick fix for add missing enum member #25182
Conversation
| @@ -0,0 +1,79 @@ | |||
| /* @internal */ | |||
| namespace ts.codefix { | |||
There was a problem hiding this comment.
i would merge this with fixAddMissingMember.ts do not see why we need to separate code fixes here.
Kingwl
force-pushed
the
fix-missing-enum-member
branch
from
d5e7ae3 to
d5268c8
Compare
| const classDeclarationSourceFile = classDeclaration.getSourceFile(); | ||
| const inJs = isSourceFileJavaScript(classDeclarationSourceFile); | ||
| const call = tryCast(parent.parent, isCallExpression); | ||
| return { token, declaration: classDeclaration, makeStatic, classDeclarationSourceFile, inJs, call }; |
There was a problem hiding this comment.
consider adding a discriminant here, e.g. kind: "enum" vs kind: "class". this way you can skip the function isEnumInfo.
| } | ||
| const enumDeclaration = find(symbol.declarations, isEnumDeclaration); | ||
| if (enumDeclaration) { | ||
| return { token, declaration: enumDeclaration, enumDeclarationSourceFile: enumDeclaration.getSourceFile() }; |
There was a problem hiding this comment.
why do you need the sourceFile? you can get that from the declaration easily.
| @@ -188,6 +211,16 @@ namespace ts.codefix { | |||
| return createCodeFixAction(fixName, changes, [makeStatic ? Diagnostics.Declare_static_method_0 : Diagnostics.Declare_method_0, token.text], fixId, Diagnostics.Add_all_missing_members); | |||
| } | |||
|
|
|||
| function getActionForEnumMemberDeclaration( | |||
There was a problem hiding this comment.
consider inlining this function in getCodeActions
| return createEnumMember(token, enumMemberInitializer); | ||
| } | ||
|
|
||
| function addEnumMemberDeclaration(changes: textChanges.ChangeTracker, checker: TypeChecker, token: Identifier, enumDeclaration: EnumDeclaration, file: SourceFile) { |
There was a problem hiding this comment.
consider merging these two functions
There was a problem hiding this comment.
i mean addEnumMemberDeclaration and createEnumMemberFromEnumDeclaration
| * value of initializer is a string literal that equal to name of enum member. | ||
| * literal enum or empty enum will not create initializer. | ||
| */ | ||
| const firstMember = firstOrUndefined(enumDeclaration.members); |
There was a problem hiding this comment.
this should be some, and not first. a literal enum can have both members, e.g.:
enum E {
a,
b = 1,
c = "ss"
}
enum E {
a,
b = 1,
c = "123"
}
enum A {
a = E.c
}
enum B {
b = A.a
}
B.ccould we have some way to handle this case? |
|
I do not think we will be able to get them all.. so for that one, my gut reaction is to make it |
| if (!addToSeen(seenNames, token.text)) { | ||
| const checker = program.getTypeChecker(); | ||
| const info = getInfo(diag.file, diag.start, checker); | ||
| if (!info || !addToSeen(seenNames, info.token.text)) { |
There was a problem hiding this comment.
So... this is not really caused by your change, but was already there, but i think this is wrong.. the key to addToSeen should not be the name of the property, but rather the the pair [container symbol, property Name]. e.g. today this fails:
class C {
method() {
this.a = 0;
}
}
class D {
method() {
this.a = 0;
}
} it gets a bit more involved if one of the two classes inherit from each other.
There was a problem hiding this comment.
I think we can handle that in a different change since it was not introduced by your change.
| */ | ||
| const hasStringInitializer = some(enumDeclaration.members, member => { | ||
| const type = checker.getTypeAtLocation(member); | ||
| return !!(type && type.flags & (TypeFlags.StringLike | TypeFlags.Enum)); |
There was a problem hiding this comment.
i would just check for stringLike here, and not enum. it can be both string or number, and since we are not handling numbers, i would leave that part off.
ghost
mentioned this pull request
Fixes #25129