Show completion for newly created package. #1191

uudashr · 2017-09-04T19:09:09Z

Previously the goListAll() cache the packages, resulting completion for newly created package not shown because the outdated cache and it quite irritating.

To takeout cache, we need to invoke go list all on demand. But the execution time is unacceptable, it will try to load packages from GOROOT, GOPATH and also user home directory. That is why cp.spawn('go', [], { env: process.env }) took so long, and by not providing the env will execute faster.

Better implementation package listing tools https://github.com/haya14busa/gopkgs. It search the packages from golang src. So I replace the go list all using gopkgs.

go list all took 4.87s
gopkgs took 0.08s

No more cache on goListAll() and it fast.
Browse package also can lookup non-installed package (based on src, not the binary)
The extension will check if the gopkgs is not the expected one and prompt as missing tool

…ion behavior

…g the same way

…istAll()

msftclas · 2017-09-04T19:09:13Z

@uudashr,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

uudashr · 2017-09-04T19:19:46Z

@ramya-rao-a check please

ramya-rao-a · 2017-09-05T03:58:27Z

src/goPackages.ts

+			let pkgs = new Map<string, string>();
+			chunks.join('').split('\n').forEach((pkgPath) => {
+				if (!pkgPath) return;
+				const lastIndex = pkgPath.lastIndexOf('/');


The main reason I used go list all instead of gopkgs initially was to get the package name and not rely on the import path of the package to determine the package name like you are doing here.

Because package names need not necessarily be the last part of the package path. See #647

And we use the package names to provide completion for unimported packages.

I see... make sense, because it completion, not import package

Yes, and later on I added the Go: Browse Packages feature because then I had all the packages in hand, so it seemed like a good feature to add

We can strip off the version part from "pkgname.v1" , it works when people always using standard way on naming package related to the package path, but that doesn't guarantee

That's exactly what I did in cb0d2bc :)

cb0d2bc no longer in master?

Here are some example, can't guarantee consistent pattern:

speech;google.golang.org/api/speech/v1beta1

spectrum;google.golang.org/api/spectrum/v1explorer

speech;google.golang.org/api/speech/v1

prediction;google.golang.org/api/prediction/v1.2

tilde;gopkg.in/mattes/go-expand-tilde.v1

So using go list -f '{{.Name}};{{.ImportPath}}' all is the proper way, it just need to make it faster.

Case

Autocomplete un-imported will suggest only packages on cache, no way to suggest new package since it need invalidation. Need to listen some events to invalidates, haven't figure out which one.

Go: Browse Packages use https://github.com/tpng/gopkgs which looks in $GOPATH/pkg, new package will not exists in $GOPATH/pkg, so browse package will not help. Except we change it, use https://github.com/haya14busa/gopkgs which search in $GOPATH/src. This is workaround, but still completion still stale.

Btw, why not change to https://github.com/haya14busa/gopkgs for Browse Packages? It seems good improvement, solve one problem.

Yes, I replaced cb0d2bc with using go list -f '{{.Name}};{{.ImportPath}}' all

I am fine with auto complete using cache as it does today.

I didnt use https://github.com/tpng/gopkgs for the Browse Packages feature as it will not have the "main" packages as in the ones that are executable

I am fine with using https://github.com/haya14busa/gopkgs for just the Browse Packages feature as well as long as it lists the "main" packages as well

uudashr · 2017-09-07T02:53:40Z

@ramya-rao-a latest changes should be able to show correct package name

uudashr · 2017-09-11T06:54:19Z

@ramya-rao-a please check

msftclas · 2017-09-11T12:50:00Z

@uudashr,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

ramya-rao-a · 2017-09-12T00:16:51Z

src/goInstallTools.ts

 function getMissingTools(goVersion: SemVersion): Promise<string[]> {
 	let keys = Object.keys(getTools(goVersion));
 	return Promise.all<string>(keys.map(tool => new Promise<string>((resolve, reject) => {
+		if (tool === 'gopkgs') {
+			gopkgsMissing().then((missing) => {


This line will be reached only when there is no gopkgs tool in the user's machine.
Whereas you are expecting to reach here to make a decision on whether the gopkgs present in the user machine is the old one or the new one

What do you think of this instead?
ramya-rao-a@2119ec1

Ahhh... I see your point. Agree with that. We can remove the gopkgsMissing() then

Ok, I have pushed my change to your branch

ramya-rao-a · 2017-09-12T05:53:50Z

@uudashr The below assert is failing when vendor is not supported. Any idea why?

https://github.com/uudashr/vscode-go/blob/faster-go-list-all/test/go.test.ts#L590

uudashr · 2017-09-12T06:00:00Z

@ramya-rao-a Not sure, still investigating...
For sure because now listPackages() use getImportablePackages().
Logic from old listPackages() not compatible with getImportablePackages()
And it happens only on Go 1.5 and 1.6

ramya-rao-a · 2017-09-12T06:42:59Z

@uudashr Tests are fixed now

ramya-rao-a · 2017-09-12T07:30:47Z

@uudashr First off, thanks for all your work in getting this together

There are a few things I still want to discuss

If gopkgs for any reason runs more than 5 seconds, then every word typed after 5 seconds will end up creating a new process. We should safeguard against this. Something like "if a current gopkgs process is running, don't start a new one"
Completions are usually expected to show up under a second. How do we ensure this when gopkgs takes more than a second? One way is to decouple the calls to gopkgs and gocode. This way even if you don't get completion for new pkg on first attempt, you will most likely get it in the second attempt.

uudashr · 2017-09-12T07:56:58Z

You are welcome... thanks for fixing the test

Cool
Have something in my mind.. something like create subscription to the currently running process if any
Something like Promise.race([goAllPackagesNoCache(), timeout(1000).then(() => cache)])

uudashr · 2017-09-12T08:45:59Z

@ramya-rao-a for point number 2, roughly will be going to be like https://gist.github.com/uudashr/8fbacf975cb8140b962fc7fd3122ff22

uudashr added 12 commits August 31, 2017 11:00

Refactor getNonVendorPackages to getPackages and respect the 1.9 vers…

6d098d6

…ion behavior

Inline the function test arguments building

ab7465f

Test all packages for Go 1.9 use "./...", the lower still remain usin…

a500a42

…g the same way

Merge remote-tracking branch 'upstream/master' into faster-go-list-all

ddc4d02

Fix autocomplete un-imported packages: show newly added package

730874a

Use spawn to avoid maxBuffer exceeded

e265725

Support large output by using spawn

75c19a3

Completions shows result instantly, no longer need to pre-call to goL…

9dadbca

…istAll()

Fix lint warning

1a4c748

Expect proper gopkgs binary

f7a75d0

Change the gopkgs package

c7e0f4f

Use spawn instead of execFile to support large output

9583a0b

msftclas added the cla-already-signed label Sep 4, 2017

ramya-rao-a reviewed Sep 5, 2017

View reviewed changes

uudashr added 4 commits September 7, 2017 08:32

Use gopkgs from http://github.com/uudashr/gopkgs

6ab865b

Merge branch 'master' into faster-go-list-all

41b1fa5

Update the go tools in .travis.yml

40b2efd

Fix the gopkgs missing tools detection

95638f7

uudashr added 4 commits September 7, 2017 11:10

Cache the gopkgs tool invocation result

805ac03

Refresh gopkgs cache

6febef2

Drop the cache for gopkgs since it already fast enough

a2a1334

Adapt with the changes of gopkgs by using -format

93379bc

uudashr closed this Sep 7, 2017

uudashr reopened this Sep 7, 2017

uudashr closed this Sep 7, 2017

uudashr reopened this Sep 7, 2017

trigger travis build: use array desctructuring assignment

fe23338

uudashr added 9 commits September 11, 2017 08:58

Add go 1.9.x to the .travis.yml

2c273e6

Prompt for missing tools when "gopkgs" not found

7d91dd8

Update the comment of goListAll

6533a93

Revert back the function name "goPackages" to "goNonVendorPackages"

8fb142e

Rename "goListAll" to "getAllPackages"

ffcb0ef

Use existing getImportablePackages

819c3db

Missing tool prompt will be handled by function caller

636efa5

Remove unused import

f57805c

Handle missing tool when browsing package

e6a39ae

uudashr added 3 commits September 11, 2017 16:46

Use cache if still used for less than 5s

a62c95c

Ignore the stderr to avoid stuck

e6f9454

Fix lint warning

e2ef816

uudashr closed this Sep 11, 2017

uudashr reopened this Sep 11, 2017

msftclas added the cla-already-signed label Sep 11, 2017

Prompt to update gopkgs if using old pkgs

2119ec1

ramya-rao-a suggested changes Sep 12, 2017

View reviewed changes

Cleanup

18cbcf8

ramya-rao-a added 2 commits September 11, 2017 23:29

Fix tests

2a898f0

Sort pkgs before testing

7a5c9db

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

View reviewed changes

ramya-rao-a merged commit e5aa763 into microsoft:master Sep 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show completion for newly created package. #1191

Show completion for newly created package. #1191

uudashr commented Sep 4, 2017 •

edited

Loading

msftclas commented Sep 4, 2017

uudashr commented Sep 4, 2017

ramya-rao-a Sep 5, 2017 •

edited

Loading

uudashr Sep 5, 2017

ramya-rao-a Sep 5, 2017

uudashr Sep 5, 2017

ramya-rao-a Sep 5, 2017

uudashr Sep 5, 2017 •

edited

Loading

ramya-rao-a Sep 6, 2017 •

edited

Loading

uudashr commented Sep 7, 2017

uudashr commented Sep 11, 2017

msftclas commented Sep 11, 2017

ramya-rao-a Sep 12, 2017 •

edited

Loading

uudashr Sep 12, 2017

ramya-rao-a Sep 12, 2017

ramya-rao-a commented Sep 12, 2017

uudashr commented Sep 12, 2017

ramya-rao-a commented Sep 12, 2017

ramya-rao-a commented Sep 12, 2017 •

edited

Loading

uudashr commented Sep 12, 2017

uudashr commented Sep 12, 2017

Show completion for newly created package. #1191

Show completion for newly created package. #1191

Conversation

uudashr commented Sep 4, 2017 • edited Loading

msftclas commented Sep 4, 2017

uudashr commented Sep 4, 2017

ramya-rao-a Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

uudashr Sep 5, 2017

Choose a reason for hiding this comment

ramya-rao-a Sep 5, 2017

Choose a reason for hiding this comment

uudashr Sep 5, 2017

Choose a reason for hiding this comment

ramya-rao-a Sep 5, 2017

Choose a reason for hiding this comment

uudashr Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

ramya-rao-a Sep 6, 2017 • edited Loading

Choose a reason for hiding this comment

uudashr commented Sep 7, 2017

uudashr commented Sep 11, 2017

msftclas commented Sep 11, 2017

ramya-rao-a Sep 12, 2017 • edited Loading

Choose a reason for hiding this comment

uudashr Sep 12, 2017

Choose a reason for hiding this comment

ramya-rao-a Sep 12, 2017

Choose a reason for hiding this comment

ramya-rao-a commented Sep 12, 2017

uudashr commented Sep 12, 2017

ramya-rao-a commented Sep 12, 2017

ramya-rao-a commented Sep 12, 2017 • edited Loading

uudashr commented Sep 12, 2017

uudashr commented Sep 12, 2017

uudashr commented Sep 4, 2017 •

edited

Loading

ramya-rao-a Sep 5, 2017 •

edited

Loading

uudashr Sep 5, 2017 •

edited

Loading

ramya-rao-a Sep 6, 2017 •

edited

Loading

ramya-rao-a Sep 12, 2017 •

edited

Loading

ramya-rao-a commented Sep 12, 2017 •

edited

Loading