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

Intentional choice to not collapse imports? #374

Closed
benclarkwood opened this issue Jul 5, 2016 · 9 comments
Closed

Intentional choice to not collapse imports? #374

benclarkwood opened this issue Jul 5, 2016 · 9 comments

Comments

@benclarkwood
Copy link
Contributor

I love the auto import features of vscode-go, but it always bugs me that I can wind up in scenarios where the imports aren't collapsed into a

import (
  // first
  // second
  // etc
)

statement instead. I'd like to change goImport.ts to handle this collapsing. I can wrap it in a setting if that is more amenable.

@ramya-rao-a
Copy link
Contributor

@benclarkwood
In a go file with no imports, I added fmt.Println("Hello") and saved to trigger the auto import which gave me

import "fmt"

Then I added math.Abs(1) and saved the file which updated the import block to

import (
   "fmt"
   "math"
)

The auto imports do get collapsed.
Is your scenario any different?

@peterbourgon
Copy link

peterbourgon commented Sep 19, 2016

@ramya-rao-a While it does work like that sometimes, I can easily get vscode-go into a state where subsequent imports are added one-by-one, i.e.

import "fmt"
import "math"
import "os"

IMO there's no reason not to use the import grouping form all the time. That is, even when I just have a single import, I want it to look like

import (
    "fmt"
)

Perhaps we can change it to simply have this behavior?

@benclarkwood
Copy link
Contributor Author

benclarkwood commented Sep 19, 2016

@ramya-rao-a sorry I didn't include proper repro steps before. Here is a repro that always causes the failure to collapse for me:

  1. Create a file (e.g. importsbreak.go) with an import:
package main

func test() {
    fmt.Println("test")
}
  1. Save and close the file (the auto-import will add fmt)
  2. Reopen the file, use the command palette Go: Add Import command to add another import (e.g. net/http)
  3. I at least end up with something like this then:
package main

import "fmt"
import "net/http"

func test() {
    fmt.Println("test")
    http.NewRequest("GET", "banana", nil)

}

As @peterbourgon suggested, the "simplest" (and arguably best/most consistent) solution is to just always use the

import (
  ...
)

block style for imports. I'll take a look at what has to change in goImport.ts for that and submit a PR against this ticket.

@ramya-rao-a
Copy link
Contributor

@benclarkwood @peterbourgon Sorry, I saw the word "goImport" and my brain mapped it to "goimports" the formatting tool and thought the problem was with the imports that get added when formatting is triggered.

At the moment, any new import added via the Go: Add Import command gets added inside () if existing imports on the page are already inside a () or if there are no imports on the existing page. Else, they get added as separate import statements.

I think this was intentional so that if someone has separate imports to begin with, the status quo is maintained.

@lukehoban Can you confirm?

You could add a setting which when true will always collapse the imports even if the current file has multiple import statements.

@peterbourgon
Copy link

peterbourgon commented Sep 19, 2016

I think this was intentional so that if someone has separate imports to begin with, the status quo is maintained.

If you have more than 1 import then you always want them grouped.

// Always this.
import (
    "fmt"
    "os"
)

// Never this.
import "fmt"
import "os"

If you have only 1 import then it can be without parens, but there's no reason it needs to be.

// This is fine.
import "fmt"

// But this is no less correct.
import (
    "fmt"
)

@benclarkwood
Copy link
Contributor Author

benclarkwood commented Sep 19, 2016

@ramya-rao-a as you said, I think the right approach is for my change to be wrapped in a setting (default == false) that when true gives @peterbourgon and I what we want (always block imports) and when false maintains the current status quo.

Which is what I shall endeavor to do :).

@benclarkwood
Copy link
Contributor Author

benclarkwood commented Sep 23, 2016

So, I took a quick (and a little bit slapdash) stab at this.

AFAICT, the only way to end up with many single line imports is:

  1. Intentionally having them
  2. Running goimports when you have exactly one package referenced, and then manually adding imports via the command palette.

With the flag enabled, both #1 and #2 should no longer occur.

The new behavior (with go.blockImportsOnly == true) is:

File with no imports:

  1. User manually adds an import from command palette
  2. The extension inserts this as import ( "package" )

File with one or more single line imports

  1. User adds an import from command Palette
  2. All imports are collapsed into a block
  3. User saves file, goimports runs and properly sorts the imports in the block

File that already has an import block

  1. User adds an import from command palette
  2. Import is appended to the block
  3. User saves file, goimports runs and properly sorts the new import into place

@ramya-rao-a
Copy link
Contributor

The linked PR has been merged and this change will be out in the next update

@ramya-rao-a
Copy link
Contributor

This is now out in the latest update to the Go extension (0.6.72)

@vscodebot vscodebot bot locked and limited conversation to collaborators Feb 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants