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

goBrowsePackages: toString bug fix + do not wait for goListAll #1136

Merged
merged 3 commits into from
Aug 17, 2017

Conversation

marwan-at-work
Copy link
Contributor

@marwan-at-work marwan-at-work commented Aug 8, 2017

This PR fixes two things:

  1. An error in goListAll() where we call chunks.toString(). The method puts ,'s between every chunk which causes some lines to not be correct imports. I think this is why the auto-import on tab didn't work for all imports (at least for me)

  2. Refactored goBrowsePackages so that we don't have to wait for goListAll to finish. This should speed up browsing packages a lot at initial time, especially for users with slower computers and very large GOPATH tree.

In goBrowsePackages we still need a feature that depends on goListAll(), which is to show a list of all the packages in your GOPATH to select from. So that logic is still there.

@msftclas
Copy link

msftclas commented Aug 8, 2017

@marwan-at-work,
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

Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good. I have 2 comments.

showPackageFiles(selectedText);
}

function showPackageFiles(pkg: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if pkg is empty string which can happen when the command is run without any editor open, then we will be needlessly creating a process to run go list

});
}

function showTryAgainLater() {
// `go list all` has not completed. Wait for a second which is an acceptable duration of delay.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if at the end of this second, go list completes, we won't be showing anything. Was that intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so that we don't get stuck in an infinite loop. And since we say try after sometime, it shouldn't be a surprise.

@ramya-rao-a ramya-rao-a merged commit 8a3f528 into microsoft:master Aug 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants