-
Notifications
You must be signed in to change notification settings - Fork 646
Add completion items for unimported packages #497
Add completion items for unimported packages #497
Conversation
return pkgInfo.name.startsWith(word); | ||
}).map((pkgInfo: PackageInfo) => { | ||
let item = new vscode.CompletionItem(pkgInfo.name, vscode.CompletionItemKind.Keyword); | ||
item.detail = 'Add import'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I needed a place to suggest that choosing this completion item would add an import and so added the item.detail
. But I guess we don't really need it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is going to need to include the pull import path name as additional context. I think being clear that this will "Add an import" is also helpful.
Just trying this out - experience is overall really nice! One thing I noticed, if you have |
Looks like you can get into cases where the completion list contains several copies of the same entry - referring to each of the fully-qualified but unique packages whose last sub-package is the selected name, as well as to the already imported name. The latter is actually a bigger problem - if I already have imported the name, I now see it twice, once based on the actual name in scope and once as a suggestion for an Add Import. The former case may require that you include the full package name in the description text. I think both of these will be important to fix - at least before making this the default experience. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few notes from a quick pass.
// TODO: Shouldn't lib-path also be set? | ||
private ensureGoCodeConfigured(): Thenable<void> { | ||
return new Promise<void>((resolve, reject) => { | ||
let pkgPromise = listPackages().then((pkgs: string[]) => { | ||
this.pkgsList = pkgs.map(pkg => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it okay for this to be fixed at startup time, not up to date as new packages are installed into the GOPATH?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didnt want to run listPackages
for every autocomplete operation and so added this basic placeholder for an actual caching mechanism. Plan was to see if users notice the missing packages and if they do, if a reload window is not too much of a hassle
return pkgInfo.name.startsWith(word); | ||
}).map((pkgInfo: PackageInfo) => { | ||
let item = new vscode.CompletionItem(pkgInfo.name, vscode.CompletionItemKind.Keyword); | ||
item.detail = 'Add import'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is going to need to include the pull import path name as additional context. I think being clear that this will "Add an import" is also helpful.
// There could be multiple words in the line | ||
// we are interested in the last one | ||
let wordmatches = null; | ||
let pattern = /(\w+)/g; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a correct regexp for Go identifies that could be packages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also - to avoid the while loop - can't you bind it to match at the end of the line? In fact, can't you avoid most of this logic by just using the regexp to match the .$
at the end of the line that you are sure is there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use /(\w+)\.$/g
and removed the while loop
Reagrding #497 (comment) I'll send out another PR for this separately |
Regarding #497 (comment) Using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Experience is much improved with the fully-qualified package name in the completion list.
I do wonder if the lack of vendor packages in the listPackages
list will be more noticeable with this feature.
I've tried this out in a few projects and I haven't seen any problems. So I think we could go ahead and merge once your happy with the changes.
It's quite a visible change, so it'll be interesting to get feedback - if there's any way to do that in advance of a new release would be great to get some users trying this out.
editBuilder.insert(new vscode.Position(lastSingleImport + 1, 0), 'import "' + imp + '"\n'); | ||
let edit = getTextEditForAddImport(imp); | ||
if (edit) { | ||
vscode.window.activeTextEditor.edit(editBuilder => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May not matter right now - but probably good to return the promise here so it get's chained into the .then
.
@@ -31,36 +31,39 @@ function askUserForImport(): Thenable<string> { | |||
}); | |||
} | |||
|
|||
export function getTextEditForAddImport(arg: string): vscode.TextEdit { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
arg
should be importPackageName
?
} | ||
}); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to remove final newlines.
Vendor packages do get returned by
I can add a setting to enable this feature, have it off by default, get feedback from users who specifically asked for this feature and anyone else you have in mind. Then, in the next release we make it a default feature.
#508 and lukehoban/go-outline#4 should take care of this. Can you take a look? |
…ature continues to work
100d889
to
e9d86d6
Compare
@lukehoban #508 has been merged, so no more double entries for the same package |
This PR adds 2 features
Assume you haven't imported
fmt
, typef
, the auto-complete will givefmt
as an option. Choose this option, the word will get completed and an import tofmt
will be added to your go fileAssume you haven't imported
math
, typemath.
, the auto-complete will give all methods frommath
package as options. Choose any, the word gets completed and an import tomath
will be added to your go fileSet
go.autocomplteUnimportedPackages
to true to enable these features.These features depend on updates to the CompletionItem API in VS Code 1.5
Fixes #407 and #437