Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

add playground command #1270

Merged
merged 9 commits into from
Nov 7, 2017
Merged

add playground command #1270

merged 9 commits into from
Nov 7, 2017

Conversation

m90
Copy link
Contributor

@m90 m90 commented Oct 7, 2017

Enables the user to upload the current file or selection to the go playground using package goplay.

This would solve #1211

@msftclas
Copy link

msftclas commented Oct 7, 2017

CLA assistant check
All CLA requirements met.

Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@m90 This is so cool :)

I have left a few comments, please take a look

outputChannel.show();
outputChannel.appendLine('Upload to the Go Playground in progress...\n');

return uploader.upload(code, config)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not make the call to goplay directly from here instead of going through the GoplayUploader ?

Copy link
Contributor Author

@m90 m90 Oct 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing that it's a common pattern in this extension to make multiple tools available (although maybe not necessarily in that case) for the same task, I thought it'd be a good idea to separate the editor part of the command from the actual uploading. Also, it makes writing tests a lot easier. I think I'd like to keep it that way if possible.

// it to the given `uploader`, together with the current editor selection
// (or the full content of the editor window if the selection is empty)
export const createCommandWith = (uploader: IPlaygroundUploader) => (): Promise<any> => {
const editor = vscode.window.activeTextEditor;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Show an error message if there is no active editor

if ((<any>err).missingTool) {
promptForMissingTool(err.missingTool);
} else {
vscode.window.showErrorMessage(err.message);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the absence of the goplay tool, we end up in the else block here with the error message write EPIPE. This is because we are passing the code via stdin and the connection gets closed due to ENOENT error

There are 2 ways to fix this

  1. Check BINARY_LOCATION is a valid absolute path. If not then dont run goplay, exit early. OR
  2. Call goplay directly from the createCommandWith function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even after installing goplay, user will have to reload VSCode for the change to take affect because the location of the binary gets calculated once in the beginning and isnt updated.

Feel free to use getBinPath for every call

? editor.document.getText()
: editor.document.getText(selection);

outputChannel.show();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clear the output channel before sending any output

@m90
Copy link
Contributor Author

m90 commented Oct 19, 2017

@ramya-rao-a Thanks for your review. I think I should have adressed all the open issues, so feel free to take another look when it fits.

@ramya-rao-a
Copy link
Contributor

it's a common pattern in this extension to make multiple tools available (although maybe not necessarily in that case) for the same task

But the playground doesn't fall into this category. There is only 1 tool that can be used.

Features in the Go extension fall into 2 categories

  1. Those that register providers and talk to VS Code apis exposed to extensions. Eg: Formatting, Completion, Definition, SignatureHelp, Hover Info, Rename etc. These follow the "implement class -> instantiate class -> use the instantiated class to register the provider" pattern
  2. Almost all other features are registering a command and providing a callback to call when the command is executed. These follow the "define a function -> export it" pattern

In the interest of keeping similar pattern in the second category here, I'd very much appreciate it if we do not use the OOP like approach for the playground feature.

Of course, I am open to consider a refactoring on a bigger scale where we move all the other features (falling in the second category) to a OOP approach. But until then, I'd prefer to keep things similar.

@m90
Copy link
Contributor Author

m90 commented Oct 23, 2017

I see your point re: consistency, yet this would mean that this turns into one untestable monolithic function (correct me if my thinking that capturing channel output in a test is not possible is wrong). I personally would feel a lot more confident shipping such a feature with unit tests, yet this is not my extension. I just wanted to raise awareness for that fact. I'll ping you once I've done the changes.

@m90
Copy link
Contributor Author

m90 commented Oct 23, 2017

@ramya-rao-a As requested I merged all the code into one function for the sake of consistency.

@ramya-rao-a ramya-rao-a merged commit f71af13 into microsoft:master Nov 7, 2017
@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Nov 7, 2017

@m90 Thanks for your work on this.
I have refactored the code a bit to enable having the unit tests you earlier had.
In case you are interested: eb8bfdb

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants