-
Notifications
You must be signed in to change notification settings - Fork 646
Sort import packages by std lib first #2803
Sort import packages by std lib first #2803
Conversation
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.
Hey @karthikraobr!
Long time no see :)
But this ordering doesn't work when the user starts entering characters to the import box.
@ramya-rao-a any pointers on how to fix this after the user starts entering characters?
We are using the quick pick control here and that control is responsible for maintaining the order after characters are being entered. I tried your code, and I see the order being maintained.
src/goImport.ts
Outdated
@@ -20,10 +20,18 @@ export async function listPackages(excludeImportedPkgs: boolean = false): Promis | |||
? await getImports(vscode.window.activeTextEditor.document) | |||
: []; | |||
const pkgMap = await getImportablePackages(vscode.window.activeTextEditor.document.fileName, true); | |||
|
|||
const stdLib = await getStandardLibraryPackages(); |
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.
Instead of making another call to go list
which we know to be expensive, why not update getImportablePackages
itself return the information on whether a package is standard or not?
Right now, we use {{.ImportPath}}
and {{.Dir}}
when calling go list
in the goPackages.ts
file.
We can add {{.Standard}}
to the same call and get a boolean representing what we need
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.
If I am not wrong I guess we are using gopkgs to list the packages and that doesn't support the {{.Standard}}
flag. That's the reason I wrote another function that uses go list
.
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.
Ah! fair point.
In that case, I would prefer if we can update gopkgs
to also support {{.Standard}}
I am guessing that whatever it does under the hood to fetch the packages, it already knows if a package is standard or not and exposing this info is a valid feature request.
Elsewhere in this project in goSuggest.ts file, a use a very crude way to identify the standard packages which is seeing if the import path contains a ".".
So, maybe until gopkgs
is fixed, we can use a similar approach?
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 will create a PR to support{{.Standard}}
in gopkgs
and then continue with this PR. That seems to be the cleanest approach right?
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.
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 see that you have used GOROOT in the Dir to determine if the package is standard or not.
We do have access to the {{.Dir}}, so maybe we can use that in the extension, compare with process.env['GOROOT']
?
src/goImport.ts
Outdated
} | ||
if (stdLib.get(b)){ | ||
return 1 | ||
} |
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.
This sorting will only ensure that the standard libraries appear before the others.
But among the standard libraries themselves, we lose the sorting
Similarly, we also lose the sorting among the non standard libraries
src/goPackages.ts
Outdated
const [pkgName, pkgPath, pkgDir] = pkgDetail.trim().split(';'); | ||
if (pkgDir.includes(process.env['GOROOT'])) { | ||
stdPkgs.set(pkgPath, pkgName); | ||
} else { | ||
otherPkg.set(pkgPath, pkgName); | ||
} | ||
}); | ||
|
||
Array.from(stdPkgs.keys()).forEach((val)=>{ | ||
pkgs.set(val, stdPkgs.get(val)); | ||
}) | ||
Array.from(otherPkg.keys()).forEach((val)=>{ | ||
pkgs.set(val, otherPkg.get(val)); | ||
}) |
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.
@ramya-rao-a This looks very crude to me :) esp maintaining two maps and merging them. Could you suggest a way to improve it? Thanks!
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.
Instead of maintaining 2 different maps, we can go back to the single map as before.
That map uses string -> string at the moment
We can change that to string -> PackageInfo where PackageInfo is an interface
export interface PackageInfo {
name string;
isStd boolean;
}
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.
This makes it extendable such that in the future if we want to extract more info from the gopkgs
tool, we can do so.
src/goPackages.ts
Outdated
const [pkgName, pkgPath, pkgDir] = pkgDetail.trim().split(';'); | ||
pkgs.set(pkgPath, { | ||
name: pkgName, | ||
isStd: pkgDir.includes(process.env['GOROOT']) |
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.
perhaps .startsWith is a better function to use here. We expect the path to start with the goroot right?
isStd: pkgDir.includes(process.env['GOROOT']) | |
isStd: pkgPath.startsWith(process.env['GOROOT']) |
Also, do we want to take care of the case where process.env['GOROOT']
doesn't have any value set? It shouldn't happen so, but if it does, do we might want to default to false here right?
src/goPackages.ts
Outdated
pkgs.set(pkgPath, pkgName); | ||
pkgs.set(pkgPath, { | ||
name: pkgName, | ||
isStd: pkgPath.includes(process.env['GOROOT']) |
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.
This is the case of the old gopkgs
. In that case, we will have to treat the output differently right?
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 am not entirely sure how the output of {{.Dir}}
would look with an earlier version?
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.
https://github.com/tpng/gopkgs is the older tool. Can you give it a try?
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.
It most likely ignores the entire formatting input
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.
Oh sorry. I forgot to try this. Will do it tomorrow.
@ramya-rao-a can you please have a look at this? Thanks |
src/goPackages.ts
Outdated
pkgs.set(pkgPath, pkgName); | ||
pkgs.set(pkgPath, { | ||
name: pkgName, | ||
isStd: goroot === null ? false : pkgPath.startsWith(goroot) |
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.
pkgPath
is not the absolute path. It is the import path. So pkgPath.startsWith(goroot)
here for the old gopkgs tool will always return false.
So, we might as well set this to false
directly.
I'll soon retire this part of the code as we don't expect anyone to be using the old gopkgs
anymore
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.
Or another option is to use this very crude logic I currently use for the auto completion widget
isStd: goroot === null ? false : pkgPath.startsWith(goroot) | |
isStd: !pkgPath.includes('.') |
I pushed a few commits to this PR @karthikraobr
|
This PR partially resolves #2683. The list is sorted by std lib packages first when there is no user input. But this ordering doesn't work when the user starts entering characters to the import box.
@ramya-rao-a any pointers on how to fix this after the user starts entering characters?