Add command to fetch import on current line by juicemia · Pull Request #1222 · microsoft/vscode-go

juicemia · 2017-09-18T04:54:16Z

This command allows users to just fetch a dependency from their editor. Under the hood, all it does is run go get for the import path on the current line.

NOTE: This is not a replacement for dependency management tools like dep.

msftclas · 2017-09-18T04:54:24Z

All CLA requirements met.

juicemia · 2017-09-18T17:37:42Z

I see that Travis is failing but I don't know why. @ramya-rao-a can you see why? I have very little experience with Travis.

ramya-rao-a · 2017-09-18T23:53:58Z

@juicemia Its just the linter failing

Run npm run lint on your branch and you will be able to see the same errors as well

ramya-rao-a · 2017-09-20T00:09:14Z

package.json

+      },
+      {
+        "command": "go.get.import",
+        "title": "Go: Get Import on Curent Line",


How about Go: Import Package? Go: Install Package ? Go: Get Package ? Go: Install Imported Package
(naming is seriously the hardest thing to do)

And the command would be more intuitive if it had the word "package" like go.get.package or go.import.package

Also update the description. It should be clear that we run a go get -u on the package

I think Go: Get Package is the best one out of all of them, because it follows the semantics of using go get. I'll change that now.

ramya-rao-a · 2017-09-20T00:11:41Z

src/goGetImport.ts

+		return vscode.window.showErrorMessage('Could not locate Go binaries. Make sure you have Go installed');
+	}
+
+	fs.exists(path.join(getCurrentGoPath(), 'src', importPath), (exists) => {


we can skip this check and run go get -u regardless
If the package already exists then it will be updated.
Thoughts?

I think it's a good idea, if we make sure to let the user know that their package will be updated. They might not want that for whatever reason.

EDIT: We can ask them to confirm if they want to update.

ramya-rao-a · 2017-09-20T00:13:49Z

src/goGetImport.ts

+				return;
+			}
+
+			vscode.window.showInformationMessage(`Successfully fetched package ${importPath}`);


What do you think of showing the progress/success/failure messages in the output pane just like the current command Go: Install/Update Tools

I think that's a great idea.

One thing I just thought of: go get -u doesn't output anything by default unless it fails. For this to be useful, we would have to run go get -u -v. We could also show the output pane only if the command fails. Thoughts?

ramya-rao-a · 2017-09-20T00:24:23Z

src/goGetImport.ts

+	const selection = editor.selection;
+	const selectedText = editor.document.lineAt(selection.active.line).text;
+
+	const importPath = getImportPath(selectedText);


If user runs this command on a line thats not an import statement, shouldnt we exit earlier? We know go get would fail then.

Yup absolutely. Forgot that check.

ramya-rao-a · 2017-09-25T21:04:51Z

test/go.test.ts

 		}).then(() => done(), done);
 	});
+
+	test('getImportPath()', () => {


may I just say that I absolutely love it that you have added tests 🥇

This will allow users to easily fetch a dependency without having to take their attention away from the code they're editing. microsoft#1189

The plugin only fetches one import so having the file name plural is misleading to anybody reading the code. microsoft#1189

This consolidates the functionality found in two commands into one place so that if it ever needs to be changed or removed it can be done more easily.

microsoft#1189

Anybody else who comes along later and reads this is gonna have an easier time if they only have to keep one name in their head vs. having to keep two (goGetImport and goGetPackage). microsoft#1189

If there's no import path to pass to `go get` then there's no point in spinning up a child process and waiting for it to finish to error out. This also fixes a bug in `getImportPath` that would cause it to return invalid input paths for things like comments and function definitions. microsoft#1189

ramya-rao-a · 2017-09-25T21:15:10Z

src/goGetPackage.ts

+		return vscode.window.showErrorMessage('Could not locate Go binaries. Make sure you have Go installed');
+	}
+
+	fs.exists(path.join(getCurrentGoPath(), 'src', importPath), (exists) => {


getCurrentGoPath() can return : or ; separated paths as well. This would fail then

Instead you can run go list and pass the importpath to see if the package exists.
Or skip this check altogether.

I really feel that if a user has explicitly run this Go: Get Package command then they would know that this would get the package using go get

I'm checking there because it runs go get -u. I can remove the check and make it just run go get without passing -u so that if the package already exists, nothing happens. It's basically the same behavior but it offloads the check to the go tooling.

ramya-rao-a · 2017-09-25T21:20:03Z

@juicemia I went ahead and rebased your branch from master as there were some conflicts. You might have to delete your local branch and checkout from the remote again.

Sorry for the trouble, I usually work alone, so didnt think of the troubles rebasing might cause others :(

juicemia · 2017-09-26T01:27:08Z

It's okay. I'll figure it out, no worries.

`go get` won't download a package if it's already in `$GOPATH/src`. There's no point in checking for the package to exist and then downloading with `-u` because `go get -u` and `go get` are effectively the same when a package hasn't been downloaded yet. microsoft#1189

`go get -v` gives users more information on what's going on with the command. This way a user can tell if the package was even downloaded, because by default `go get` won't output anything. `go get -v` will output nothing if the package isn't downloaded because it already exists. To get around this, an information message is shown saying that the package wasn't downloaded. This keeps the user informed because they will be expecting feedback from the command. In the event of an error (like if there's no internet connection), the error gets shown to the output pane so the user knows what happened. microsoft#1189

ramya-rao-a · 2017-09-28T02:05:25Z

Thanks @juicemia!

juicemia force-pushed the fetch-imports branch from 77f9f7f to cb9d11c Compare September 20, 2017 00:07

ramya-rao-a suggested changes Sep 20, 2017

View reviewed changes

ramya-rao-a reviewed Sep 20, 2017

View reviewed changes

ramya-rao-a reviewed Sep 25, 2017

View reviewed changes

Hugo Torres added 7 commits September 25, 2017 14:08

Add command to fetch import on current line

2c3d6b8

This will allow users to easily fetch a dependency without having to take their attention away from the code they're editing. microsoft#1189

Rename goGetImports to goGetImport

55ac72c

The plugin only fetches one import so having the file name plural is misleading to anybody reading the code. microsoft#1189

Refactor getImportPath

e17e0a5

This consolidates the functionality found in two commands into one place so that if it ever needs to be changed or removed it can be done more easily.

Fix lint issues

b1e1f67

Make command title and description more intuitive

ba45d15

microsoft#1189

Make name goGetPackage consistent

640b1f4

Anybody else who comes along later and reads this is gonna have an easier time if they only have to keep one name in their head vs. having to keep two (goGetImport and goGetPackage). microsoft#1189

ramya-rao-a force-pushed the fetch-imports branch from 9ac522f to 8473a3d Compare September 25, 2017 21:10

Fallback to selectedText for browsePackage feature

5893c90

ramya-rao-a reviewed Sep 25, 2017

View reviewed changes

Hugo Torres and others added 3 commits September 27, 2017 20:59

Cleanup

061befe

ramya-rao-a approved these changes Sep 28, 2017

View reviewed changes

ramya-rao-a merged commit 2e2e3cd into microsoft:master Sep 28, 2017

ramya-rao-a mentioned this pull request Aug 16, 2018

support to execute "go get xxx" in context menu when the package can't be found. #716

Closed

Conversation

juicemia commented Sep 18, 2017 • edited by ramya-rao-a Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented Sep 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juicemia commented Sep 18, 2017

Uh oh!

ramya-rao-a commented Sep 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juicemia Sep 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramya-rao-a commented Sep 25, 2017

Uh oh!

juicemia commented Sep 26, 2017

Uh oh!

ramya-rao-a commented Sep 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

juicemia commented Sep 18, 2017 •

edited by ramya-rao-a

Loading

msftclas commented Sep 18, 2017 •

edited

Loading

juicemia Sep 23, 2017 •

edited

Loading