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

Prioritize current project import suggestions #1782

Merged
merged 5 commits into from
Jul 18, 2018

Conversation

marwan-at-work
Copy link
Contributor

This PR will make it so that "import suggestions" will always put your project's packages at the top. This really helps when there are very common package names like log or errors and you want to import them through the autocomplete feature. Before this pr, you'd have to scroll through 10s if not 100s of packages to find the one form you're workspace.

From the screenshot below, you can see that if you're inside upspin.io's sourcode, then the autocomplete shows you imports from that package first.
screen shot 2018-07-09 at 11 11 48 pm

src/goSuggest.ts Outdated
@@ -346,6 +346,7 @@ export class GoCompletionItemProvider implements vscode.CompletionItemProvider {
private getMatchingPackages(word: string, suggestionSet: Set<string>): vscode.CompletionItem[] {
if (!word) return [];
let completionItems = [];
const rootPath = vscode.workspace.rootPath.slice(getCurrentGoPath().length + 5);
Copy link
Contributor

Choose a reason for hiding this comment

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

This will fail in a few cases

  • Multi-root mode. To fix this you can use vscode.workspace.getWorkspaceFolder(fileUri)
  • The file being edited is not part of the folder opened in VS Code or the file alone has been opened in VS Code. No folders. In this case vscode.workspace.getWorkspaceFolder(fileUri) would not return a workspace, so you can fall back to the parent folder of the file.
  • The file being edited is not part of the gopath
  • The getCurrentGopath returns multiple gopaths like projectA:projectB or projectA;projectB. To solve this you can use getCurrentGoWorkspaceFromGOPATH method from goPath.ts

src/goSuggest.ts Outdated
@@ -363,9 +364,15 @@ export class GoCompletionItemProvider implements vscode.CompletionItemProvider {
// Add same sortText to the unimported packages so that they appear after the suggestions from gocode
const isStandardPackage = !item.detail.includes('.');
item.sortText = isStandardPackage ? 'za' : 'zb';
completionItems.push(item);
if (pkgPath.startsWith(rootPath)) {
item.sortText = 'a';
Copy link
Contributor

@ramya-rao-a ramya-rao-a Jul 16, 2018

Choose a reason for hiding this comment

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

If all the packages share the same sortText, I believe that the sort order then is the order that they appear in the completionItems array. Then why change the sortText here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ramya-rao-a this is so that the unimported packages from the same project (or workspace) are prioritized over the rest of the package suggestions. Say you are in project a and you start typing b in main.go, when the imports are suggested i'd like to see a/b before someOtherProject/b so i can just hit enter and a/b is imported. Most of the time when you wanna import a package, it's most likely either from your own project, or from the standard library, or a third party. Third party libraries have special cool names so they usually come first anyway. But when you type generic package name like log or errors, you get too many suggestions from GOPATH and you most likely just want the log package from your own project.

Copy link
Contributor

Choose a reason for hiding this comment

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

Take a hypothetical case

  • lab, lag, large, lamb are standard pkgs
  • github.com/mine/log, github.com/you/log are custom packages

The user is currently working on the github.com/mine/myproject and types l
The suggestions would now appear as

  • log -> github.com/mine/log
  • lab
  • lag
  • large
  • lamb
  • log -> github.com/you/log

This would be counter intutive, as all the std pkgs would be expected to be sorted above alphabetically.

The solution to your problem should be such that your package is sorted to the top but only among other packages with the same name. So taking the same example as above, typing l should result in

  • lab
  • lag
  • large
  • lamb
  • log -> github.com/mine/log
  • log -> github.com/you/log

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ramya-rao-a ah, yes that was intentional. If you have a log package inside your project, then you are most likely going to import your own log package than the standard library one.

But I'm still happy if the requirement is that standard packages are always shown first.

Which way do you prefer? And if the latter, how would you like me to make the sorting work? I'm thinking something like:

if (isStandardLibrary) sortLetter = 'a';
else if (isProjectLibrary) sortLetter = 'b';

would that work? assuming we have a function to figure out if an import is std lib :)

Copy link
Contributor

Choose a reason for hiding this comment

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

My previous comment assumed that the unimported package suggestions were sorted among themselves alphabetically. Which is not the case. My bad.

How about item.sortText = isStandardPackage ? 'za' : pkgPath.startsWith(rootPath) ? 'zb' : 'zc'?

We need the z as we dont want any of the unimported packages to appear before other normal symbol suggestions.

@marwan-at-work
Copy link
Contributor Author

@ramya-rao-a i updated and commented the pr, thanks for taking a look!

@marwan-at-work
Copy link
Contributor Author

@ramya-rao-a I've applied your last suggested edit, which seems to be working as intended. Thanks again for pointing out all the edge cases! Let me know if this needs further edits.

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.

@marwan-at-work I pushed a commit with slight re-factoring. Can you test it from your end? Then, I'll merge the PR.

@marwan-at-work
Copy link
Contributor Author

@ramya-rao-a tested it out. Looks good. Thank you!!

@marwan-at-work
Copy link
Contributor Author

@ramya-rao-a ok to merge on my end.

Something kind of related but probably for another pr: would it be possible to just remove all packages nested under GOPATH/src/mod from suggestions and imports? This is because of vgo (and soon Go 1.11) new module system. We're never gonna import anything directly from there.

I'm happy to push another PR for it if you can give me some direction. Thanks!

@ramya-rao-a
Copy link
Contributor

There is an ongoing discussion for vgo in #1532. Add you note there with some links to documentation and we can take it from there.

@ramya-rao-a ramya-rao-a merged commit 5f43e98 into microsoft:master Jul 18, 2018
@ramya-rao-a
Copy link
Contributor

This fix is now out in the latest update (0.6.85) to the Go extension.

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.

2 participants