You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Add a RenameProvider to support rename refactor #31
Hey - I took a stab at it, but didn't ping this issue. Happy to hold off on my implementation. @ikhwanhayat, feel free to mine bits of it out that are helpful.
In VS Code for TypeScript, it just opens all the affected file. For undo, you need to undo in each of the files individually. My implementation just follows that.
This sounds reasonable to me then. Maybe in future Code will support prompting etc. for us.
Not quite sure on how to handle error messages though.
What you've done all looks good to me; what's your concern with this? If it's displaying all the messages, maybe just show the first (that's what Devon's did). Depending on the messages, we might need to display them differently though.
If you open a PR when you think it's basically working, I'll have a test and see how it feels.
What you've done all looks good to me; what's your concern with this? If it's displaying all the messages, maybe just show the first (that's what Devon's did). Depending on the messages, we might need to display them differently though.
I am thinking whether these kind of messages are better shown using the popup alert (vscode.window.showErrorMessage(...)) or is it better if can create a "Refactor" output channel that we can standardize to be used for all kind of refactorings.
I think displaying the first error message and failing the refactor would be fine. It'll give the user enough info about why the refactor couldn't happen. They can then address the issue (if it's something the user can fix) and try and do the rename again.
The number of error levels may be a bit overspecified. The API was designed for a client which supports displaying a preview of the changes to the user.
My gut would be to ignore info level items and fail on warning or error ones. You might also file a bug against the analysis server to improve the docs about what the different severity levels mean - when you might get a particular severity, and the danger in just continuing with the refactoring.
Regarding weird rename behavior if done on reserved words, I notice similar behavior happens too for Go To Definition and Find References.
If let's say I have import 'something.dart' as something;, I put my caret at import, when I hit Find References it will get me all references to something.
For var f = new something.Foo();, when I put caret on var or new and do a Go To Definition, it shows the definition for something and for Foo.
So I guess this is normal behavior for Analysis Server?
It is normal behaviour for the analysis server, but it's not very compatible with how Code displays this. For Go to Def I think that behaviour is fine, nothing is really misleading. For rename, it's prepopulating a box with something wrong and that results in a change the user doesn't want and them having to undo it.
Eg.:
var f =newSomething();
If I put my cursor on Something and hit F2 I get a rename box populated with Something. This looks like it's letting me rename the class, so I type SomethingElse. Now I end up with a constructor on Something called SomethingElse and the code looks like new Something.SomethingElse(). The only thing that's going to happen is Ctrl+Z!
Given this looks really buggy to the end user (it doesn't matter if we can't fix it, it looks like a bug in our code) and that Code already has an alternative to this for the simple case (Ctrl+F2 does a word-based rename) I'd like to improve before shipping.
If MS will implement microsoft/vscode#7340 then I think we can fix it one way or another even without support from the AS (though I really think the AS should be improved; the current behaviour here seems really wonky to me for all IDEs unless).
All that said, if not having this is a real pain, I wouldn't be so against shipping it disabled with an option to enable. We could add it to release notes with an explanation of why it's opt-in to ensure anybody using it/finding it's bugs is aware of the status?
We can probably also improve this without the VS Code change (though it means rejecting the rename after the uesr already typed it).
My idea is to use one of the other AS data sets that has something like an AST we can use to figure out if we're on a real import or constructor call to block them (basically a more accurate version of the keyword blocklist).
Ok, I believe I have a reliable workaround for this! :)
The AS returns us the old name when renaming something. We can compare this against the word under the cursor. For now, this means we can reliably abort renames that are confusing, and once microsoft/vscode#7340 is done we can issue a dummy request and use the oldName field to pre-populate the box (this will give us "perfect" behaviour, but with slight latency and it's hacky). I've raised #144 for improving this when we get Code support.
I've also raised dart-lang/sdk#27178 as a request for the AS to better support this (not require us to send a dummy rename).
Just doing some more testing and then I'll push this and I'd appreciate testing/feedback :-)
@ikhwanhayat@devoncarew Right, I have merged this to master! Please test and raise cases if you notice any unexpected behaviour.
For now, if you try to perform a misleading rename (eg. on the call to a constructor, an import, whatever) where Code would pre-populate the box incorrectly, the rename will be aborted.
Once Code is updated to allow us control over the F2 behaviour, we will fix this for good (#144).
Thanks @ikhwanhayat for the work, and sorry it took so long to merge in; I appreciate it's frustrating, but I really didn't want to ship something that looked really buggy; I think the current behaviour is much more understandable (and with the Code changes I think it'll be perfect, even if there are no AS changes).
@ikhwanhayat@devoncarew Just pushed a possible bugfix... I thought it worked before, but I found empty strings for oldNames weren't trigger the error. So, if you've pulled these changes to look at already, you might want to update again.
(Note: haven't merged into all other branches yet, test on master).
Activity
ikhwanhayat commentedon Aug 14, 2016
Just a heads up, I'm currently doing this.
DanTup commentedon Aug 14, 2016
Looks like @devoncarew may have started on this yesterday; not sure of the status though .. ?
32aeba4
devoncarew commentedon Aug 14, 2016
Hey - I took a stab at it, but didn't ping this issue. Happy to hold off on my implementation. @ikhwanhayat, feel free to mine bits of it out that are helpful.
implement rename
DanTup commentedon Aug 15, 2016
@ikhwanhayat:
This sounds reasonable to me then. Maybe in future Code will support prompting etc. for us.
What you've done all looks good to me; what's your concern with this? If it's displaying all the messages, maybe just show the first (that's what Devon's did). Depending on the messages, we might need to display them differently though.
If you open a PR when you think it's basically working, I'll have a test and see how it feels.
Thanks!
ikhwanhayat commentedon Aug 15, 2016
I am thinking whether these kind of messages are better shown using the popup alert (
vscode.window.showErrorMessage(...)) or is it better if can create a "Refactor" output channel that we can standardize to be used for all kind of refactorings.DanTup commentedon Aug 15, 2016
Not sure; maybe worth seeing what others do. I haven't actually tried to do this in TypeScript yet despite all the coding in it recently!
devoncarew commentedon Aug 15, 2016
I think displaying the first error message and failing the refactor would be fine. It'll give the user enough info about why the refactor couldn't happen. They can then address the issue (if it's something the user can fix) and try and do the rename again.
ikhwanhayat commentedon Aug 15, 2016
RefactoringProblemSeverity can be either INFO, WARNING, ERROR, or FATAL
So I guess, if it's an INFO or WARNING we just continue with the refactor anyway.
But in my tests, I've only encountered severity of level ERROR. I tested:
devoncarew commentedon Aug 15, 2016
The number of error levels may be a bit overspecified. The API was designed for a client which supports displaying a preview of the changes to the user.
My gut would be to ignore info level items and fail on warning or error ones. You might also file a bug against the analysis server to improve the docs about what the different severity levels mean - when you might get a particular severity, and the danger in just continuing with the refactoring.
DanTup commentedon Aug 15, 2016
I agree with Devon; let's treat warnings as aborts for now.
I've raised dart-lang/sdk#27078 asking for clarification in the docs.
5 remaining items
ikhwanhayat commentedon Aug 28, 2016
Regarding weird rename behavior if done on reserved words, I notice similar behavior happens too for Go To Definition and Find References.
If let's say I have
import 'something.dart' as something;, I put my caret atimport, when I hit Find References it will get me all references tosomething.For
var f = new something.Foo();, when I put caret onvarornewand do a Go To Definition, it shows the definition forsomethingand forFoo.So I guess this is normal behavior for Analysis Server?
DanTup commentedon Aug 28, 2016
It is normal behaviour for the analysis server, but it's not very compatible with how Code displays this. For Go to Def I think that behaviour is fine, nothing is really misleading. For rename, it's prepopulating a box with something wrong and that results in a change the user doesn't want and them having to undo it.
Eg.:
If I put my cursor on
Somethingand hitF2I get a rename box populated withSomething. This looks like it's letting me rename the class, so I typeSomethingElse. Now I end up with a constructor onSomethingcalledSomethingElseand the code looks likenew Something.SomethingElse(). The only thing that's going to happen isCtrl+Z!Given this looks really buggy to the end user (it doesn't matter if we can't fix it, it looks like a bug in our code) and that Code already has an alternative to this for the simple case (
Ctrl+F2does a word-based rename) I'd like to improve before shipping.If MS will implement microsoft/vscode#7340 then I think we can fix it one way or another even without support from the AS (though I really think the AS should be improved; the current behaviour here seems really wonky to me for all IDEs unless).
All that said, if not having this is a real pain, I wouldn't be so against shipping it disabled with an option to enable. We could add it to release notes with an explanation of why it's opt-in to ensure anybody using it/finding it's bugs is aware of the status?
DanTup commentedon Aug 28, 2016
We can probably also improve this without the VS Code change (though it means rejecting the rename after the uesr already typed it).
My idea is to use one of the other AS data sets that has something like an AST we can use to figure out if we're on a real
importor constructor call to block them (basically a more accurate version of the keyword blocklist).Remove keyword blacklist.
DanTup commentedon Aug 28, 2016
Ok, I believe I have a reliable workaround for this! :)
The AS returns us the old name when renaming something. We can compare this against the word under the cursor. For now, this means we can reliably abort renames that are confusing, and once microsoft/vscode#7340 is done we can issue a dummy request and use the oldName field to pre-populate the box (this will give us "perfect" behaviour, but with slight latency and it's hacky). I've raised #144 for improving this when we get Code support.
I've also raised dart-lang/sdk#27178 as a request for the AS to better support this (not require us to send a dummy rename).
Just doing some more testing and then I'll push this and I'd appreciate testing/feedback :-)
DanTup commentedon Aug 28, 2016
@ikhwanhayat @devoncarew Right, I have merged this to master! Please test and raise cases if you notice any unexpected behaviour.
For now, if you try to perform a misleading rename (eg. on the call to a constructor, an import, whatever) where Code would pre-populate the box incorrectly, the rename will be aborted.
Once Code is updated to allow us control over the
F2behaviour, we will fix this for good (#144).Thanks @ikhwanhayat for the work, and sorry it took so long to merge in; I appreciate it's frustrating, but I really didn't want to ship something that looked really buggy; I think the current behaviour is much more understandable (and with the Code changes I think it'll be perfect, even if there are no AS changes).
DanTup commentedon Aug 28, 2016
@ikhwanhayat @devoncarew Just pushed a possible bugfix... I thought it worked before, but I found empty strings for
oldNamesweren't trigger the error. So, if you've pulled these changes to look at already, you might want to update again.(Note: haven't merged into all other branches yet, test on master).
devoncarew commentedon Aug 28, 2016
🎉