Use new 'goimport' tool instead of VimScript for :GoImport and :GoDrop

[arp242]

Instead of a large regexp-based VimScript to parse imports,
it now uses goimport, which parses the ast.

Fixes #1534 and deals with various other edge cases too. The behaviour is mostly the same, except in cases where it didn't make too much sense to keep the old behaviour.

For example, :GoImport errors will now error out if errors is already imported, and :GoImport github.com/pkg/errors will now replace any existing errors package instead of just adding it (this is actually the reason I started on this, because it's pretty annoying otherwise).

[codecov-io]

Codecov Report

Merging #1559 into master will increase coverage by 0.99%.
The diff coverage is 78.12%.

@@            Coverage Diff            @@
##           master   #1559      +/-   ##
=========================================
+ Coverage    14.8%   15.8%   +0.99%     
=========================================
  Files          53      53              
  Lines        4153    4055      -98     
=========================================
+ Hits          615     641      +26     
+ Misses       3538    3414     -124

Flag	Coverage Δ
#nvim	15.48% <78.12%> (+0.99%)	⬆️
#vim74	15.73% <78.12%> (+0.99%)	⬆️
#vim80	15.46% <75%> (+0.96%)	⬆️

Impacted Files	Coverage Δ
plugin/go.vim	29.26% <ø> (ø)	⬆️
ftplugin/go/commands.vim	0% <0%> (ø)	⬆️
autoload/go/import.vim	78.78% <83.33%> (+78.78%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40e2e19...b26f493. Read the comment docs.

[arp242]

Okay, finished up the loose ends and this seems to work well now.

[arp242]

I maintained the current commands for compatibility btw, but we could remove :GoImportAs and simply make the second argument for :GoImport optional.

[bhcleek]

Using the AST is going to fail when the go file is not yet parseable; e.g. if someone is in the middle of editing it and they :GoImport time. Consider trying to use :GoImport when the go file is

package main

func main() {
   showTime()
}

func showTime() error {
    fmt.Println(time.Now())

Given that GoImport is intended to help while editing, I don't think :GoImport should require a file that is syntactically correct...

[arp242]

Using the AST is going to fail when the go file is not yet parseable

Not exactly; we pass the parser.ImportsOnly flag, which means it will only parse the import block. There are probably some limits to what it can and can't parse, but in general it works fairly well with invalid syntax (including your example).

[fatih]

Albeit less people use this because of goimports, I still think this is valuable. I'm +1 on this

For example, :GoImport errors will now error out if errors is already imported,

I don't think this should show an error, instead it should be a noop. Just like :GoFmt, :GoAddTags, etc..

[bhcleek]

@Carpetsmoker testing reveals :GoImport io will panic when run

package foo

import()

func bar() {
    println("quux")
}

And if it will remove the package declaration if run in

package foo

func bar()
    println("quux")
}

[bhcleek]

I haven't tested it yet, but I definitely want to make sure this works with build tags before merging.

[arp242]

I fixed the panic; I'm not 100% sure what you mean with "And if it will remove the package declaration if run in"; is that related to the panic or another issue?

It works fine with build tags, as it operates on files rather than packages. I added a test for it to make sure.

[bhcleek]

It's related to another issue. When the file does not have an import statement, I was seeing the :Goimport cause the package declaration to be elided.

[arp242]

Oh right; that should be fixed too now.

[bhcleek]

I found another scenario where this fails. I created two new tests: refs/pull/1559/head...bhcleek:pr-1559/more-tests

The Test_import_noemptyline one fails.

[fatih]

@Carpetsmoker Martin, what's the state of this? Let us know how you feel about this.