-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Pvb/refactor/jsdoc #11951
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/refactor/jsdoc #11951
Conversation
Thanks for your contribution. This PR has failing CI tests and can not be merged in at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to merge this code in, please open a new PR that has been merged and rebased with the |
|
||
const fileTextChanges: FileTextChanges[] = []; | ||
const namePos: number = variableDeclaration.name.pos; | ||
let variableInitializerText: string = variableDeclaration.initializer.getText(); |
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 not going to work. the text from the initializer is not guaranteed to work in all places where the variable would be used. the initializer expression can contain names that have been shadowed later on and will need to be qualified accordingly . consider:
namespace N {
export var x = 0;
var temp = x;
function inner(x: string) {
console.log(temp);
}
}
renaming temp
to x
is not correct, since x
has a different meaning in this scope.
looking back at this PR, the infrastructure for refactoring has been added in #15569. the two additional refactorings added here do not seem salvageable either. I am not sure we even need to add a new JSDoc as a refactor, and inline temp needs to address in lining an expression correctly; this is not a simple change, and seems like a complete rewrite. closing this PR. |
Adds a refactoring to add jsdoc to classes/modules.
Builds on top of #11913