Proposal to implement collapsing by benclarkwood · Pull Request #500 · microsoft/vscode-go

benclarkwood · 2016-09-23T21:59:29Z

Add new setting "go.blockImportsOnly"
Add parsing of package name for single import lines
When the user adds an import manually and there is already one or more single imports, collapse it all into a block if go.blockImportsOnly is true.

note: addresses #374

benclarkwood · 2016-09-24T01:01:23Z

package.json

          "default": "",
          "description": "The Go build tags to use for all commands that support a `-tags '...'` argument"
+        },
+        "go.blockImportsOnly": {


If anyone has a better name...

I actually think we can just make this the default. No need for a new setting here.

lukehoban

Generally looks good. Couple of comments inline.

lukehoban · 2016-09-24T23:56:52Z

src/util.ts

 		if (line.match(/^(\s)*import(\s)+[^\(]/)) {
-			ret.imports.push({ kind: 'single', start: i, end: i });
+			// Pull the package name
+			let match = line.match(/import "(.+)"/);


Need to make sure this never fails. Probably better to just modify the if match to capture the name as well? I ran into failures when a line looked like import "byte" for example (two spaces instead of one).

This will also fail in case of dot imports and named imports. Example:

import . "fmt" import m "math"

lukehoban · 2016-09-24T23:58:04Z

package.json

          "default": "",
          "description": "The Go build tags to use for all commands that support a `-tags '...'` argument"
+        },
+        "go.blockImportsOnly": {


I actually think we can just make this the default. No need for a new setting here.

lukehoban · 2016-09-24T23:59:59Z

src/goImport.ts

+				// First, delete all the single packages.
+				editBuilder.delete(new vscode.Range(new vscode.Position(firstImport.start, 0), new vscode.Position(lastImport.end, 10000)));
+				// Now, insert the import block.
+				editBuilder.insert(new vscode.Position(pkg.start + 2, 0), importBlock);


Is anchoring this on pkg.start always safe? What if there are comments in front of the imports or two groups of parenthesized imports? Can this just be anchored at firstImport.start instead?

Like @lukehoban says, this would result in comments that were previously between packages and imports to appear after the new import block.
Also, comments in between single import statements are lost too. Example:

import "math" //Helps me in math // This is my custom package import "mycustom"

How about doing this instead?

Insert (\n\t after the word "import" in the first import

Replace the word "import" with \t for rest of the imports

Insert ) in the line after the last import

benclarkwood · 2016-09-26T15:36:05Z

Oh my gosh this is such a better way of doing this!

Thanks!

On Sep 25, 2016, at 23:03, Ramya Rao notifications@github.com wrote:

@ramya-rao-a commented on this pull request.

In src/goImport.ts:
      let firstImport = imports[0];
    let lastImport = imports[imports.length - 1];
    // Construct the import block.
    let importBlock = 'import (\n';
    imports.forEach(element => {
        importBlock += '\t"' + element.package + '"\n';
    });
    importBlock += '\t"' + imp + '"\n'; // Add the user's new import
    importBlock += ')\n';
    return vscode.window.activeTextEditor.edit(editBuilder => {
        // First, delete all the single packages.
        editBuilder.delete(new vscode.Range(new vscode.Position(firstImport.start, 0), new vscode.Position(lastImport.end, 10000)));
        // Now, insert the import block.
        editBuilder.insert(new vscode.Position(pkg.start + 2, 0), importBlock);
Like @lukehoban says, this would result in comments that were previously between packages and imports to appear after the new import block.
Also, comments in between single import statements are lost too. Example:
import "math" //Helps me in math
// This is my custom package
import "mycustom"
How about doing this instead?

Insert (\n\t after the word "import" in the first import

Replace the word "import" with \t for rest of the imports

Insert ) in the line after the last import

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

ramya-rao-a · 2016-09-26T17:56:26Z

src/goImport.ts

 }

-export function addImport(arg: string) {
+export function addImport(arg: string, goConfig: vscode.WorkspaceConfiguration) {


you shouldn't be needing goConfig here anymore

ramya-rao-a · 2016-09-26T18:19:18Z

src/goImport.ts

+				editBuilder.insert(new vscode.Position(firstImport.start, 0), 'import (\n');
+
+				imports.forEach(element => {
+					editBuilder.replace(new vscode.Range(new vscode.Position(element.start, 0), new vscode.Position(element.start, importLength)), '\t');


Starting from 0 will cause issues when there are spaces/tabs before the word "import".
This shouldn't happen if formatting has been run on the file, but there might be cases where user has not yet saved the file or has turned formatOnSave off or has not formatted the doc at all

Currently parseFilePrelude returns start and end lines. You can update that to return startCharacter as well and use that

ramya-rao-a · 2016-09-26T18:20:59Z

src/goImport.ts

+				// A comment
+				import . "dotinclude"
+				import "apackage2" // A comment explaining the package
+


Can you add a unit test for this?

benclarkwood · 2016-10-06T00:01:27Z

Haven't abandoned this, just been supes busy at work. Will add tests / address additional concerns this weekend hopefully.

ramya-rao-a · 2016-10-06T23:50:33Z

@benclarkwood There has been some changes to goImport.ts recently, so make sure to get latest from master before you continue

mattetti · 2017-01-27T04:47:26Z

This PR is stuck in a conflict, @benclarkwood would you mine rebasing?
On a separate note, I find the property name go.blockImportsOnly quite confusing since it reads like we are blocking imports only. Maybe go.groupedImportsOnly?

1. Add new setting "go.blockImportsOnly" 2. Add parsing of package name for single import lines 3. When the user adds an import manually and there is already one or more single imports, collapse it all into a block if go.blockImportsOnly is true.

1. Fix linter issues

1. Back out package name parsing 2. Remove setting from package.json (always use block imports) 3. Switch up how block import is created to preserve comments and special imports

ramya-rao-a · 2018-01-08T05:01:45Z

@benclarkwood I took the liberty of rebasing and adding tests

benclarkwood · 2018-01-09T23:10:05Z

@ramya-rao-a thanks! Sorry, Golang became not my daily driver and I totally forgot about this PR. 🙇 .

msftclas added the cla-already-signed label Sep 23, 2016

benclarkwood commented Sep 24, 2016

View reviewed changes

lukehoban reviewed Sep 25, 2016

View reviewed changes

lukehoban mentioned this pull request Sep 25, 2016

Go: Add Import command allows import for packages that are already imported #494

Closed

ramya-rao-a reviewed Sep 26, 2016

View reviewed changes

ramya-rao-a force-pushed the master branch 2 times, most recently from 0b2155b to 7aa5605 Compare November 21, 2016 04:01

ramya-rao-a force-pushed the master branch 2 times, most recently from d5299a5 to 38331b7 Compare February 3, 2017 03:23

msftgits removed the cla-already-signed label Sep 26, 2017

microsoft deleted a comment from msftclas Sep 27, 2017

ramya-rao-a force-pushed the master branch 2 times, most recently from 7259f77 to f6936fa Compare October 2, 2017 20:31

ramya-rao-a force-pushed the master branch 3 times, most recently from 89e20b7 to 3d8a1f0 Compare November 4, 2017 19:44

ramya-rao-a force-pushed the master branch 2 times, most recently from 7d591f1 to a8a68e2 Compare November 19, 2017 17:39

Ben Wood and others added 4 commits January 7, 2018 20:46

With this commit:

9cf132a

1. Add new setting "go.blockImportsOnly" 2. Add parsing of package name for single import lines 3. When the user adds an import manually and there is already one or more single imports, collapse it all into a block if go.blockImportsOnly is true.

WTC:

967354f

1. Fix linter issues

Remove unused import

93f340a

With this commit:

3732c07

1. Back out package name parsing 2. Remove setting from package.json (always use block imports) 3. Switch up how block import is created to preserve comments and special imports

Post rebase cleanup

4ca4925

Add tests

a085194

ramya-rao-a merged commit 3790d5f into microsoft:master Jan 8, 2018

Conversation

benclarkwood commented Sep 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukehoban left a comment

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!

benclarkwood commented Sep 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramya-rao-a Sep 26, 2016 • 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!

benclarkwood commented Oct 6, 2016

Uh oh!

ramya-rao-a commented Oct 6, 2016

Uh oh!

mattetti commented Jan 27, 2017

Uh oh!

ramya-rao-a commented Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benclarkwood commented Jan 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ramya-rao-a Sep 26, 2016 •

edited

Loading

ramya-rao-a commented Jan 8, 2018 •

edited

Loading