Add Import to Workspace #1745

freman · 2018-06-20T04:39:24Z

Make it possible to add the path for the selected import, or part there of to the current workspace for convenience.

Resolves #1733

msftclas · 2018-06-20T04:39:35Z

All CLA requirements met.

ramya-rao-a · 2018-06-20T06:03:19Z

Thanks for the PR @freman

Any reason to prompt the user to choose among the imported packages in current document rather than say choose the package under the cursor like we do for the Go: Get Package command?

freman · 2018-06-20T13:55:47Z

Hi @ramya-rao-a no reason really, I was just excited to be getting a feature that worked around the macos path selection box.

It did have the benefit of working no matter where one was in the file.

I've taken your suggestion and expanded on it, it'll now use the current selected import line, or if the user has selected text it'll try to use that (reasoning being you might be importing foo/bar/package but it makes more sense to work in foo/bar)

It'll also try to prevent adding redundant workspaces, one future improvement I could see is removing redundant workspaces (ie you add foo/bar/package but then later add foo/bar it might be worth removing foo/bar/package automatically)

freman · 2018-06-28T04:38:42Z

Errm, I don't think I broke travis... did I?

ramya-rao-a

Sorry for taking a while to get to this PR.
I have left a few comments, please do take a look.

ramya-rao-a · 2018-06-29T00:36:01Z

src/goImport.ts

+
+	let importPath = '';
+	if (selection.isSingleLine) {
+		// Attempt to load a partial import path based on currently selected text


Can you provide an example for the case that you are attempting to cover here?

Absolutely. The easiest example that comes to mind.

import "github.com/aws/aws-sdk-go/service/rds/rdsutils"

Realistically being able to select github.com/aws/aws-sdk-go and add that to the workspace is probably going to be what you want more than github.com/aws/aws-sdk-go/service/rds/rdsutils

Good point. I would change the if (selection.isSingleLine) to if (!selection.isEmpty).
There could be a package with a very long import path or someone with a very zoomed in window.
Also trim the selected text before adding the " in the beginning and end. Else the selected text could be "github.com/golang/example" and this code here will change it to " "github.com/golang/example"

ramya-rao-a · 2018-06-29T00:41:34Z

src/goImport.ts

+
+	let filePath = fixDriveCasingInWindows(editor.document.fileName);
+	let fileDirPath = path.dirname(filePath);
+	let currentWorkspace = getCurrentGoWorkspaceFromGOPATH(getCurrentGoPath(), fileDirPath);


Take the case of multiple gopaths. Someone might have GOPATH set to /Users/folderA;/Users/folderB.
The current workspace is under /Users.folderA
The package in question could be under /Users/folderB
In this case, the current logic wont work.

How about passing the import path to go list -f {{.Dir}}? This would return the actual path for the package. You wouldnt need to to a fs.exists check either.

See showPackageFiles function in goBrowsePackage.ts for a reference on how to use go list.

True, I'll sort this out.

ramya-rao-a · 2018-06-29T00:43:00Z

src/goImport.ts

+	const existingWorkspaceFolder = getRootWorkspaceFolder(importPathUri);
+	if (existingWorkspaceFolder !== undefined) {
+		vscode.window.showInformationMessage('Already available under ' + existingWorkspaceFolder.name);
+		return;


What happens when we pass in a uri for an already existing folder to vscode.workspace.updateWorkspaceFolders? If updateWorkspaceFolders gracefully handles this case, then we dont need to do this check beforehand

I can't remember if it did or didn't, I think my motivation was to let the user know why nothing happened. I can double check but I still think it's nice to let the user know that the workspace is already imported.

IE: they try to add github.com/aws/aws-sdk-go/service/rds/rdsutils after having github.com/aws/aws-sdk-go loaded.

In an ideal world I'd actually make the workspace navigate/expand the path but I've no idea how to do this in vscode

I double checked.

Adding a subfolder of a folder in already in the work space just creates a new folder pointing to that subfolder in the workspace.

eg.

if you already have github.com/aws/aws-sdk-go/ then activate the command on github.com/aws/aws-sdk-go/service/rds/rdsutils you end up with aws-sdk-go and rdsutils as workspace folders.

Ideally I'd have it navigate to and highlight the added package but I have no idea how to do that.

ramya-rao-a · 2018-06-29T00:43:41Z

src/goMain.ts

@@ -274,6 +274,10 @@ export function activate(ctx: vscode.ExtensionContext): void {
 		return addImport(typeof arg === 'string' ? arg : null);
 	}));

+	ctx.subscriptions.push(vscode.commands.registerCommand('go.import.workspace', (arg: string) => {


What is the scenario where an arg would be passed via the command?

Uh that's a bit of left over code, it probably wouldn't at this point, I'll remove that.

ramya-rao-a · 2018-06-29T00:45:08Z

package.json

@@ -154,6 +154,10 @@
        "title": "Go: Add Import",
        "description": "Add an import declaration"
      },
+      {
+        "command": "go.import.workspace",
+        "title": "Go: Add import to workspace"


The name is not very self explanatory....
How about go.add.package.workspace and Go: Add Package to Workspace ?

freman · 2018-07-09T04:58:45Z

@ramya-rao-a is there any way to trigger travis without pushing again?

ramya-rao-a · 2018-07-16T23:24:03Z

src/goImport.ts

+	}
+
+	const goRuntimePath = getBinPath('go');
+	const env = Object.assign({}, process.env, { GOPATH: getCurrentGoPath() });


Use getToolsEnvVars()

ramya-rao-a · 2018-07-16T23:35:25Z

src/util.ts

+	let workspaceFolder = vscode.workspace.getWorkspaceFolder(forPathUri);
+	let previousWorkspaceFolder: vscode.WorkspaceFolder;
+	while (workspaceFolder !== undefined) {


Can you explain the scenario that would need this loop?
Taking your example of aws, if github.com/aws/aws-sdk-go is already part of the workspace and we run the command for github.com/aws/aws-sdk-go/service/rds/rdsutils, wouldnt vscode.workspace.getWorkspaceFolder(forPathUri) give you the uri of the parent workspace folder? And isnt that enough?

ramya-rao-a

Looks good. I have a left a few more comments. Please take a look.

freman · 2018-07-17T01:26:05Z

Thanks @ramya-rao-a, I've addressed those, the original thought behind that loop is lost to time, I suspect I just didn't understand how that call worked

ramya-rao-a

Awesome, thanks for your patience with me :)

freman mentioned this pull request Jun 20, 2018

Feature request: Add import folder to workspace #1733

Closed

freman force-pushed the feat-1733 branch from 77212b3 to 0edeba0 Compare June 20, 2018 04:54

freman changed the title ~~Initial work on adding an import folder to workspace~~ Add Import to Workspace Jun 22, 2018

ramya-rao-a suggested changes Jun 29, 2018

View reviewed changes

ramya-rao-a reviewed Jul 16, 2018

View reviewed changes

ramya-rao-a suggested changes Jul 16, 2018

View reviewed changes

Shannon Wynter and others added 4 commits July 17, 2018 11:19

Initial work on adding an import folder to workspace

a6cc2c3

Work from selection rather than prompting user

2b44ebc

Address feedback

c133b8e

Address more comments

2c4cdd8

freman force-pushed the feat-1733 branch from 56f9648 to 2c4cdd8 Compare July 17, 2018 01:23

ramya-rao-a approved these changes Jul 17, 2018

View reviewed changes

ramya-rao-a merged commit 81e7053 into microsoft:master Jul 17, 2018

freman deleted the feat-1733 branch July 17, 2018 23:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Import to Workspace #1745

Add Import to Workspace #1745

freman commented Jun 20, 2018 •

edited

Loading

msftclas commented Jun 20, 2018 •

edited

Loading

ramya-rao-a commented Jun 20, 2018

freman commented Jun 20, 2018 •

edited

Loading

freman commented Jun 28, 2018

ramya-rao-a left a comment

ramya-rao-a Jun 29, 2018

freman Jul 2, 2018

ramya-rao-a Jul 16, 2018 •

edited

Loading

ramya-rao-a Jun 29, 2018

freman Jul 2, 2018

ramya-rao-a Jun 29, 2018

freman Jul 2, 2018

freman Jul 2, 2018 •

edited

Loading

ramya-rao-a Jun 29, 2018

freman Jul 2, 2018

ramya-rao-a Jun 29, 2018

freman commented Jul 9, 2018

ramya-rao-a Jul 16, 2018

ramya-rao-a Jul 16, 2018

ramya-rao-a left a comment

freman commented Jul 17, 2018

ramya-rao-a left a comment

Add Import to Workspace #1745

Add Import to Workspace #1745

Conversation

freman commented Jun 20, 2018 • edited Loading

msftclas commented Jun 20, 2018 • edited Loading

ramya-rao-a commented Jun 20, 2018

freman commented Jun 20, 2018 • edited Loading

freman commented Jun 28, 2018

ramya-rao-a left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya-rao-a Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freman Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freman commented Jul 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya-rao-a left a comment

Choose a reason for hiding this comment

freman commented Jul 17, 2018

ramya-rao-a left a comment

Choose a reason for hiding this comment

freman commented Jun 20, 2018 •

edited

Loading

msftclas commented Jun 20, 2018 •

edited

Loading

freman commented Jun 20, 2018 •

edited

Loading

ramya-rao-a Jul 16, 2018 •

edited

Loading

freman Jul 2, 2018 •

edited

Loading