"Generate 'get' and 'set' accessors."

You can potentially keep it as "getter" and "setter", but I don't know how well that localizes. I think the key thing I have in mind is that you should have the text "generate" in there somehow.

It'd be nice if we could detect whether other public members use modifiers instead of injecting one.

Give a short comment explaining why you want to do this check.

Also, use charCodeAt(0) === CharacterCodes._.

Also, break this into at least two lines and use braces please.

Also, does this work for computed properties? You probably want to make sure that your propertyDeclaration.name is an identifier. That way you could also use propertyDeclaration.name.text after you use a type-guard.

I don't like that you have to write private _foo: number; to get this refactor. Wouldn't it make more sense to trigger on public foo: number; and convert it to private _foo: number; public get foo() {} public set foo() {}?

updated

Also, does this work for computed properties? You probably want to make sure that your propertyDeclaration.name is an identifier. That way you could also use propertyDeclaration.name.text after you use a type-guard.

You will actually need to make sure that you create a unique name that doesn't exist in the current class. For simplicity, I would just bail out if the property with this name already exists.

Same deal with propertyDeclaration.name.text here and below.

If the initial property access is protected then this should be protected. The inner variable should be private

Nit: please keep file casing consistent by renaming the file and this reference to it. (first letter should be lower-case.

updateoriginalPropertyDeclaration

Seems problematic to drop decorators here.

Also strange to drop the token.

originName to originalName

Put this on multiple lines.

I believe we have a range API instead, but I haven't ever written one of these tests yet, so this might be fine?

i'm not sure...
I copied it from the refactor Convert Es6 Module

Hmm, I think I initially meant whether a field had a public modifier explicitly

what if the field is Protected

i do not see why we would not allow this. you just need to come up with a temporary name that starts with _a, set the rename location to it, and let the user decide what they want to call it. you can just call createUniqueName to create you an identifier with a unique name and use it instead.

See https://github.com/Microsoft/TypeScript/blob/1ed30c6d4d78aeb7de0c81604dbf38120d82e82b/src/services/refactors/extractSymbol.ts#L1137-L1161

why? i would expect this to be get _a

newLineCharacter should now be available on ChangeTracker.newLineCharacter , no need to recompute it,

what about static? you can have static accessors

we also need to handle parameter properties. so this can be a property or a parameter declaration.

instead of these two lines, you can just check that isClassLike(propertyDeclaration.parent) or isClassLike(parameterDeclaration.parent.parent)

do not think you need that. as i noted in my other comment, we should pick a unique name that has a seed of "_" + <property name>, then set the rename location on this name. the user will get the chance to rename it right there and then.

so in short the name should always be updated.

do not think you need these either.

i would say you do not need to create this type until you need to execute the refactoring. this function is called in both getAvailableActions and getEditsForAction, so ideally we would defer that until we are computing the edits.

nit, keep the [ and { on the same line.

also keep in mind that propertyDeclaration.type may not be there. if this is the case, you want to not include a type, nor do you want to merge it with undefined.

also you should add a test for that.

Variable can be inlined

Prefer members.some(...) over find when just using as a boolean

Also, .getText() on every member might be slow since getText() has to do scanning. Might want to do something like function getDeclarationName from services.ts to just get Identifier and StringLiteral-named properties.

Also, getMemberName(member.name) === (needUpdateName ? fieldName : accessorName)

Actually, can avoid looping over members and instead check for checker.getPropertyOfType(checker.getTypeAtLocation(containerClass.name), needUpdateName ? fieldName : accessorName) which avoids conflicts with supertypes too.

Isn't this just containerClass.members?

hasModifiers currently is true if any member on the class has modifiers. Instead this could just preserve whatever modifier was there before on the particular member without looking at other members. I.e., public x: number becomes public get x() { ... } public set x() { ... } and x: number becomes get x() { ... } set x() { ... }, making hasModifiers unnecessary.

needUpdateName and needUpdateModifiers aren't used separately, so I would combine them to one property.

Nit: would move this closer to its use.

With the latest changes to ChangeTracker it shouldn't be necessary to explicitly specify options. If it is, file an issue and we can fix it later after this PR is merged.

*modifiers

Might want to make const originalName early since it's used in 3 other places.

Mohamed Hegazy (@mhegazy) some thing need help
in this case
how should i add the , after every props
if i update the ObjectLiteralExpression, the original PropertyAssignment will overship with this change

what should i do with this type declaration,
should i get the type with typeChecker?

no, you are fine. the type will be inferred from the return type of the getter.

you still want to handel the static modifier in the case of js. i would jsut make it

return createNodeArray(
             append<Modifier>(isJS ? [] : [createToken(SyntaxKind.PrivateKeyword)],
                 isStatic ? createToken(SyntaxKind.StaticKeyword) : undefined));