[WIP] JS refactor->rename for named variables, members and functions in a single file #13047
Conversation
|
|
||
|
|
||
| // Only provide rename when cursor is in JavaScript content | ||
| if (!session || session.editor.getModeForSelection() !== "javascript") { |
There was a problem hiding this comment.
Can session.editor be null or undefined? Most likely not, but might be worth checking out.
|
|
||
| function requestFindRefs(session, document, offset) { | ||
| var path = document.file.fullPath, | ||
| fileInfo = {type: MessageIds.TERN_FILE_INFO_TYPE_FULL, |
There was a problem hiding this comment.
Nit: Have we used some kind of style for multiline objects? The indentation looks a bit weird, I would prefer
fileInfo = {
type: MessageIds.TERN_FILE_INFO_TYPE_FULL,
name: path,
offsetLines: 0,
text: filterText(session.getJavascriptText())
};| function requestFindRefs(session, offset) { | ||
| var response = ScopeManager.requestFindRefs(session, session.editor.document, offset); | ||
|
|
||
| if (response.hasOwnProperty("promise")) { |
There was a problem hiding this comment.
Is there are case where response wouldn't have a promise? requestFindRefs looks like it's pretty null safe, but might be good to guard with response && response.hasOwn...
| var response = ScopeManager.requestFindRefs(session, session.editor.document, offset); | ||
|
|
||
| if (response.hasOwnProperty("promise")) { | ||
| response.promise.done(handleFindRefs).fail(function () { |
There was a problem hiding this comment.
Can we use then and catch to align the old Deferred syntax with the more modern Promises/A+ syntax?
| * on the response. | ||
| */ | ||
| handleFindRefs = function (refsResp) { | ||
| function isInSameFile(obj) { |
There was a problem hiding this comment.
Can be shortened to return obj && obj.file === refsResp.file
| } | ||
| } | ||
|
|
||
| if (refsResp && refsResp.refs && refsResp.refs.refs) { |
There was a problem hiding this comment.
Random comment: refsResp.refs.refs looks kinda silly 👯
| _log("Error returned from Tern 'definition' request: " + error); | ||
| return; | ||
| } | ||
| var response = {type: MessageIds.TERN_REFS, |
There was a problem hiding this comment.
NIT: Same thing here with the new lines and objects
|
Thanks a lot @petetnt for the quick review. I will try to address the comments asap. I would urge all of you to use the feature as well and let me know if it's helpful. |
| @@ -329,5 +329,8 @@ | |||
| ], | |||
| "help.support": [ | |||
| "F1" | |||
| ], | |||
| "refactor.rename": [ | |||
| "Ctrl-R" | |||
There was a problem hiding this comment.
I believe on macOS this is translated to Cmd+R which is used to reload Brackets with Extensions.
| @@ -867,6 +923,8 @@ define(function (require, exports, module) { | |||
|
|
|||
| return response; | |||
| } | |||
|
|
|||
| CommandManager.register("Rename", "refactor.rename", handleRename); | |||
There was a problem hiding this comment.
You should add the string in command/Commands.js instead of using the string explicit.
|
From the code, this seems to work only for javascript files. I assume languages that derive from javascript in CodeMirror are NOT supported (for example json, typescript, ...) |
|
@ficristo Yes, it's targeted for javascript mode. So if CM identifies any block of code as javascript, this would work there. That includes script blocks in html, any modes derived from JS by CodeMirror. BTW, is there any extension available today which implements this feature? |
|
@swmitra Can you post a gif of how this works? |
|
@abose I am not able to post a gif right now as I dont have access to any computer for a few days. But what you can do is - put the cursor on top of the token you want to rename. Then press ctrl+r . This will put selection on all the occurrences after doing a scope analysis. Now just type the new name. You will see multi cursor in action and should be able to rename all the occurrences. |
|
👍 Cool feature |
This PR implements "rename" functionality in JS mode to enable intelligent rename feature by using Terns 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. Once the other PR #11948 is merged, we can leverage on the JS analysis in node and include all files to build symbol table.
The rename function works by selecting a variable def/ref or placing cursor in and using Ctrl+R.
Know issues
In a particular case Tern inference engine is picking up wrong hit. For example -
function fn1(error) {error.a = "abc";var a = "test";console.log(a);}If we try to rename
ain line 3 or 4 , Tern picks up fake ref from line 2 as well.Yet to be done