-
Notifications
You must be signed in to change notification settings - Fork 7.6k
JavaScript Refactoring - Rename Identifier, Wrap selection, Convert to arrow function, Create Getters & Setters #13965
Conversation
…ode domain, popping up error at cursor
👍 @navch |
Is it OK to squash all of your commits into one single commit? |
var templates = JSON.parse(require("text!Templates.json")); | ||
|
||
/** | ||
* Note - Don't use this if you are in a batch operation and you already made some changes |
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 entire comment section needs to be rewritten.
offset, handleFindRefs; | ||
|
||
if (editor.getSelections().length > 1) { | ||
editor.displayErrorMessageAtCursor("Rename doesn't work in case of multicursor"); |
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 string needs to be externalized. And correct the wording as well.
|
||
//Do rename of identifier which is at cursor | ||
function handleRename() { | ||
var editor = EditorManager.getActiveEditor(), |
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.
Are you going to allow rename inside quick edit session?
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.
Yes. Rename is working great in quick edit as well for local variables. Renaming function calls could be a problem. According to me we should allow rename in quick edit as well. Please let me know if want me to disable rename in quick edit.
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.
OK. I think it is better to disable rename for quick edit. There are chances that once you invoke a quick edit on a function name, we only show the contents of the function. But there may be variables whose scope is outside the function and I am not sure how you are going to show the feedback for renaming variable references which are not shown inside quick edit.
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.
Let us do this. I will merge this PR. But please raise another PR addressing this concern.
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.
Sure @nethip I will do that. Thanks for suggestion.
var funcExprNode = current.findSurroundASTNode(current.ast, {start: current.startIndex}, ["FunctionExpression"]); | ||
|
||
if (!funcExprNode || funcExprNode.type !== "FunctionExpression" || funcExprNode.id) { | ||
current.editor.displayErrorMessageAtCursor("Cursor is not inside function expression"); |
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.
String needs to be externalized.
@navch All error popup strings need to go into |
Thanks @nethip for reviewing this. I addressed all review comments. |
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.
Few review comments
* file for cursor | ||
*/ | ||
function getRefs(fileInfo, offset) { | ||
var request = buildRequest(fileInfo, "refs", offset); |
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.
Can fileInfo
be null or undefined ever?
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 should not be the case. Anyways I handled this when we are creating fileInfo.
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 will add template at given position/selection | ||
* | ||
* @param {string} template - name of templated defined in templates.json |
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.
Nit: name of the template
* | ||
* @param {string} template - name of templated defined in templates.json | ||
* @param {Array} args- Check all arguments that exist in defined templated pass all that args as array | ||
* @param {{line: number, ch: number}} rangeToReplace - Range which we want to rreplace |
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.
Nit: replace
//Post message to tern node domain that will request tern server to find refs | ||
function getRefs(fileInfo, offset) { | ||
ScopeManager.postMessage({ | ||
type: "getRefs", |
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.
type: MessageIds.TERN_REFS,
maybe
* If yes then select all references | ||
*/ | ||
handleFindRefs = function (refsResp) { | ||
function isInSameFile(obj) { |
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.
isInSameFile
could be externalized from the function
* Check if references are in this file only | ||
* If yes then select all references | ||
*/ | ||
handleFindRefs = function (refsResp) { |
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.
Maybe cleaner if this one was assigned inline above requestFindReferences
?
function handleFindRefs(refsResp)...
function requestFindReferences(session, offset) { | ||
var response = requestFindRefs(session, session.editor.document, offset); | ||
|
||
if (response || response.hasOwnProperty("promise")) { |
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 should probably be if (response && response.hasOwnProperty("promise")
?
src/nls/root/strings.js
Outdated
"CMD_REFACTORING_CONDITION" : "Wrap in Condition", | ||
"CMD_REFACTORING_GETTERS_SETTERS" : "Create Getters Setters", | ||
"CMD_REFACTORING_ARROW_FUNCTION" : "Convert to Arrow Function", | ||
"DESCRIPTION_CODE_REFACTORIG" : "Enable/disable JavaScript Code Refactoring", |
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.
DESCRIPTION_CODE_REFACTORING
|
||
// This preference controls whether to create a session and process all JS files or not. | ||
PreferencesManager.definePreference("refactoring.JSRefactoring", "boolean", true, { | ||
description: Strings.DESCRIPTION_CODE_REFACTORIG |
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.
DESCRIPTION_CODE_REFACTORING
…nor change in templates.json
Thanks @petetnt for reviewing this. I addressed all review comments. |
This PR implements "rename" functionality in JS mode to enable intelligent rename feature by using Tern's scope analysis and inference functions. Renaming across multiple files has been intentionally blocked for the time being as we are not analyzing all the JS files in current project root right now.
The rename function works by selecting a variable def/ref or placing cursor in and using Ctrl+R.
This code is written by @swmitra and already reviewed by @petetnt (PR 13047)
I just restructured this code and created new extension called JavascriptCodeRefactoring.
Known issues:-
In a particular case Tern inference engine is picking up wrong hit. For example :-
If we try to rename a in line 3 or 4 , Tern picks up fake ref from line 2 as well.
#Issue reported in tern 950, 951
951 is already fixed.
Wrap Selection in Try Catch/Condition
else if you selected code then it will check weather these are statements or not
else error (Please select valid statements)
AppInit.appReady(function () { brackets.test.doneLoading = true; });
Select whole code or just place cursor on 3rd line it will wrap whole code in try catch block/Condition block
try { AppInit.appReady(function () { brackets.test.doneLoading = true; }); } catch (e) { console.log(e.message); }
Put selection on 2nd line and do wrap in try catch/condition it will wrap that line only.
Know Issue:- Some time indention goes wrong in case of wrap in condition
Convert to arrow function
If one param structure will be
param => {statements}
If zero or more than one param
(param1, param2) => {statements}
In case of one statement, it will remove "{, }" and ";"as well
Create Setters and Getters for given property
get getTest() { return this.test; }, set setTest(val) { this.test = val; }
Please review @nethip @boopeshmahendran @petetnt @swmitra @ficristo @saurabh95