-
Notifications
You must be signed in to change notification settings - Fork 646
Add command to extract method out of selected code #370
Add command to extract method out of selected code #370
Conversation
Hi @ramya-rao-a, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
TTYL, MSBOT; |
This is a cool feature/tool @ramya-rao-a! Would you mind checking why the build is failing? It might also be nice to add this feature to the context menu now that it's available to extensions. |
Looks like something is wrong with the cert used by
|
Thanks @mattetti . Once I get this PR merged, I'll get to the context menu. Need to set the same via travis |
return editPromise.then(done => { | ||
if (done && !editor.document.getText().endsWith('\n')) { | ||
return editor.edit((editBuilder) => { | ||
editBuilder.insert(new Position(editor.document.lineCount, 0), '\n'); |
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 you add the \n
if necessary on line 74? Also this should probably insert the same line endings as are used in the document (\r\n
or \n
).
process.stdin.end(editor.document.getText() + '\n');
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.
@Tyriar Extracted methods get added at the end of the document. If \n
gets added to the input, then the resulting diffs/patches will assume this \n
exists and not output one. This will result in the extracted method getting appended to the last line instead of starting from a new line after the last line.
I'll be doing a remote meetup during GopherCon this week and will demo VS Code, I'd love to show off this feature :) |
I've looked at the code and it LGTM. @mattetti we are having some struggle with the publishing of this extension. For the meetup can you try to side-load the extension?
|
@egamma it turned out the internet connection at the conference wasn't good enough for the live streaming so we postponed the event. I however showed off VS Code to many people, especially because there was a talk on Guru (used by VS Code) and Delve (the debugger used by Code). Lots of people were very impressed by how fast and feature full VS Code for Go is. I'll side-load master and show the refactoring function tomorrow during the hack day. Thanks a lot |
One small detail: make sure to restart Code (I forgot the first time around ;)) A few things about this new feature, while it's the promise of this refactoring tool is really exciting, the practical use case seems to be pretty limited. I couldn't extract code with return statements, I saw weird errors of undefined types (no idea what was going on) and finally this interesting error: The error is interesting because it's meaningless for the user, I have not idea what code is on line 201 of the stdin buffer, and no idea why it ended being an error. My guess is that my extracted code was added as a function but the new code wasn't valid (it needed some massaging?), so the underlying tool failed with this cryptic error. @ramya-rao-a did you have good luck using this refactoring tool? It's highly possible I got very unlucky and the code I was trying to refactor wasn't a good example. |
I just had a chance to try out this feature. In the 4 projects I tried it in, and half dozen locations trying to extract method in each, I didn't actually find a single case where it worked :-). Are we sure that GoDoctor is robust enough for this integration, and if so, that the integration is working sufficiently often that this feature is a net win for the extension? Here are a few of the errors I saw during an Extract Method:
I also agree with @mattetti that the last of these makes this hard to use in practice in Go, since return statements are so common in error checking interspersed through Go function bodies. Also, in most of the projects I tried, these changes appear to have broken formatting with The issues with |
I talked with my friend Fatih who develops and maintains go-vim. He told me I am excited about the opportunity of having a great refactoring/function On Sat, Jul 16, 2016, 12:01 Luke Hoban notifications@github.com wrote:
|
godoctor does have limitations in the extract method feature.
@mattetti My examples did not contain any of the above and the refactoring did work. @lukehoban I didn't have format on save set, so couldnt catch the Regardless, since it seems like godoctor doesn't extract the method well in many common scenarios like you both have experienced, I will revert the changes for now. |
Thanks @ramya-rao-a, and sorry for not reviewing this sooner! It would be great to reopen this PR with the fixes for formatting so that folks can try out the Extract Method feature and provide feedback that may help inform whether GoDoctor could be further improved to be a useful extension. |
@lukehoban I have reopened the PR after fixing the issue with formatting. #404 Can you provide me sample go code where you found the below errors? It would help me determine if the problem is with godoctor or my integration.
Thanks! |
Related to a feature ask in #275
This commit adds a command to the command palette that can be used to extract a method out of selected code and replace the selected code with a call to the extracted method using godoctor. Read more about godoctor at http://gorefactor.org/