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

Sort import packages by std lib first #2803

Merged
merged 10 commits into from
Nov 4, 2019
23 changes: 19 additions & 4 deletions src/goImport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,27 @@ export async function listPackages(excludeImportedPkgs: boolean = false): Promis
? await getImports(vscode.window.activeTextEditor.document)
: [];
const pkgMap = await getImportablePackages(vscode.window.activeTextEditor.document.fileName, true);

return Array.from(pkgMap.keys())
.filter(pkg => !importedPkgs.some(imported => imported === pkg))
.sort();
.sort((a, b) => {
if (pkgMap.get(a).isStd && pkgMap.get(b).isStd) {
if (a < b) {
return -1
}
if (a > b) {
return 1
}
return 0
} else if (pkgMap.get(a).isStd) {
return -1
} else if (pkgMap.get(b).isStd) {
return 1
}
Copy link
Contributor

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

return 0
});
}


/**
* Returns the imported packages in the given file
*
Expand Down Expand Up @@ -100,15 +115,15 @@ export function getTextEditForAddImport(arg: string): vscode.TextEdit[] {
}
}

export function addImport(arg: {importPath: string, from: string}) {
export function addImport(arg: { importPath: string, from: string }) {
const p = (arg && arg.importPath) ? Promise.resolve(arg.importPath) : askUserForImport();
p.then(imp => {
/* __GDPR__
"addImportCmd" : {
"from" : { "classification": "SystemMetaData", "purpose": "FeatureInsight" }
}
*/
sendTelemetryEvent('addImportCmd', { from: (arg && arg.from) || 'cmd'});
sendTelemetryEvent('addImportCmd', { from: (arg && arg.from) || 'cmd' });
const edits = getTextEditForAddImport(imp);
if (edits && edits.length > 0) {
const edit = new vscode.WorkspaceEdit();
Expand Down
60 changes: 35 additions & 25 deletions src/goPackages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,17 @@ import { promptForMissingTool, promptForUpdatingTool } from './goInstallTools';
import { envPath, fixDriveCasingInWindows, getCurrentGoWorkspaceFromGOPATH } from './goPath';
import { getBinPath, getCurrentGoPath, getGoVersion, getToolsEnvVars, isVendorSupported, sendTelemetryEvent } from './util';

type GopkgsDone = (res: Map<string, string>) => void;
type GopkgsDone = (res: Map<string, PackageInfo>) => void;
interface Cache {
entry: Map<string, string>;
entry: Map<string, PackageInfo>;
lastHit: number;
}

export interface PackageInfo {
name: string;
isStd: boolean;
}

let gopkgsNotified: boolean = false;
let cacheTimeout: number = 5000;

Expand All @@ -26,16 +31,17 @@ const allPkgsCache: Map<string, Cache> = new Map<string, Cache>();

const pkgRootDirs = new Map<string, string>();

function gopkgs(workDir?: string): Promise<Map<string, string>> {

function gopkgs(workDir?: string): Promise<Map<string, PackageInfo>> {
const gopkgsBinPath = getBinPath('gopkgs');
if (!path.isAbsolute(gopkgsBinPath)) {
promptForMissingTool('gopkgs');
return Promise.resolve(new Map<string, string>());
return Promise.resolve(new Map<string, PackageInfo>());
}

const t0 = Date.now();
return new Promise<Map<string, string>>((resolve, reject) => {
const args = ['-format', '{{.Name}};{{.ImportPath}}'];
return new Promise<Map<string, PackageInfo>>((resolve, reject) => {
const args = ['-format', '{{.Name}};{{.ImportPath}};{{.Dir}}'];
if (workDir) {
args.push('-workDir', workDir);
}
Expand All @@ -48,7 +54,7 @@ function gopkgs(workDir?: string): Promise<Map<string, string>> {
cmd.stderr.on('data', d => errchunks.push(d));
cmd.on('error', e => err = e);
cmd.on('close', () => {
const pkgs = new Map<string, string>();
const pkgs = new Map<string, PackageInfo>();
if (err && err.code === 'ENOENT') {
return promptForMissingTool('gopkgs');
}
Expand All @@ -64,7 +70,7 @@ function gopkgs(workDir?: string): Promise<Map<string, string>> {
console.log(`Running gopkgs failed with "${errorMsg}"\nCheck if you can run \`gopkgs -format {{.Name}};{{.ImportPath}}\` in a terminal successfully.`);
return resolve(pkgs);
}

const goroot = process.env['GOROOT']
const output = chunks.join('');
if (output.indexOf(';') === -1) {
// User might be using the old gopkgs tool, prompt to update
Expand All @@ -75,19 +81,23 @@ function gopkgs(workDir?: string): Promise<Map<string, string>> {
}
const index = pkgPath.lastIndexOf('/');
const pkgName = index === -1 ? pkgPath : pkgPath.substr(index + 1);
pkgs.set(pkgPath, pkgName);
pkgs.set(pkgPath, {
name: pkgName,
isStd: goroot === null ? false : pkgPath.startsWith(goroot)
Copy link
Contributor

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

Copy link
Contributor

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

Suggested change
isStd: goroot === null ? false : pkgPath.startsWith(goroot)
isStd: !pkgPath.includes('.')

});
});
return resolve(pkgs);
}

output.split('\n').forEach((pkgDetail) => {
if (!pkgDetail || !pkgDetail.trim() || pkgDetail.indexOf(';') === -1) {
return;
}
const [pkgName, pkgPath] = pkgDetail.trim().split(';');
pkgs.set(pkgPath, pkgName);
const [pkgName, pkgPath, pkgDir] = pkgDetail.trim().split(';');
pkgs.set(pkgPath, {
name: pkgName,
isStd: goroot === null ? false : pkgDir.startsWith(goroot)
});
});

const timeTaken = Date.now() - t0;
/* __GDPR__
"gopkgs" : {
Expand All @@ -102,10 +112,10 @@ function gopkgs(workDir?: string): Promise<Map<string, string>> {
});
}

function getAllPackagesNoCache(workDir: string): Promise<Map<string, string>> {
return new Promise<Map<string, string>>((resolve, reject) => {
function getAllPackagesNoCache(workDir: string): Promise<Map<string, PackageInfo>> {
return new Promise<Map<string, PackageInfo>>((resolve, reject) => {
// Use subscription style to guard costly/long running invocation
const callback = function(pkgMap: Map<string, string>) {
const callback = function (pkgMap: Map<string, PackageInfo>) {
resolve(pkgMap);
};

Expand Down Expand Up @@ -134,7 +144,7 @@ function getAllPackagesNoCache(workDir: string): Promise<Map<string, string>> {
* @argument workDir. The workspace directory of the project.
* @returns Map<string, string> mapping between package import path and package name
*/
export async function getAllPackages(workDir: string): Promise<Map<string, string>> {
export async function getAllPackages(workDir: string): Promise<Map<string, PackageInfo>> {
const cache = allPkgsCache.get(workDir);
const useCache = cache && (new Date().getTime() - cache.lastHit) < cacheTimeout;
if (useCache) {
Expand Down Expand Up @@ -163,32 +173,32 @@ export async function getAllPackages(workDir: string): Promise<Map<string, strin
* @param useCache. Force to use cache
* @returns Map<string, string> mapping between package import path and package name
*/
export function getImportablePackages(filePath: string, useCache: boolean = false): Promise<Map<string, string>> {
export function getImportablePackages(filePath: string, useCache: boolean = false): Promise<Map<string, PackageInfo>> {
filePath = fixDriveCasingInWindows(filePath);
const fileDirPath = path.dirname(filePath);

let foundPkgRootDir = pkgRootDirs.get(fileDirPath);
const workDir = foundPkgRootDir || fileDirPath;
const cache = allPkgsCache.get(workDir);

const getAllPackagesPromise: Promise<Map<string, string>> = useCache && cache
const getAllPackagesPromise: Promise<Map<string, PackageInfo>> = useCache && cache
? Promise.race([getAllPackages(workDir), cache.entry])
: getAllPackages(workDir);

return Promise.all([isVendorSupported(), getAllPackagesPromise]).then(([vendorSupported, pkgs]) => {
const pkgMap = new Map<string, string>();
const pkgMap = new Map<string, PackageInfo>();
if (!pkgs) {
return pkgMap;
}

const currentWorkspace = getCurrentGoWorkspaceFromGOPATH(getCurrentGoPath(), fileDirPath);
pkgs.forEach((pkgName, pkgPath) => {
if (pkgName === 'main') {
pkgs.forEach((info, pkgPath) => {
if (info.name === 'main') {
return;
}

if (!vendorSupported || !currentWorkspace) {
pkgMap.set(pkgPath, pkgName);
pkgMap.set(pkgPath, info);
return;
}

Expand All @@ -208,7 +218,7 @@ export function getImportablePackages(filePath: string, useCache: boolean = fals

const allowToImport = isAllowToImportPackage(fileDirPath, currentWorkspace, relativePkgPath);
if (allowToImport) {
pkgMap.set(relativePkgPath, pkgName);
pkgMap.set(relativePkgPath, info);
}
});
return pkgMap;
Expand Down Expand Up @@ -301,4 +311,4 @@ function isAllowToImportPackage(toDirPath: string, currentWorkspace: string, pkg
return toDirPath.startsWith(rootProjectForInternalPkg + path.sep) || toDirPath === rootProjectForInternalPkg;
}
return true;
}
}
13 changes: 7 additions & 6 deletions src/goSuggest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import cp = require('child_process');
import { getTextEditForAddImport } from './goImport';
import { promptForMissingTool, promptForUpdatingTool } from './goInstallTools';
import { isModSupported } from './goModules';
import { getImportablePackages } from './goPackages';
import { getImportablePackages,PackageInfo } from './goPackages';
import { getCurrentGoWorkspaceFromGOPATH } from './goPath';
import { byteOffsetAt, getBinPath, getCurrentGoPath, getGoConfig, getParametersAndReturnType, getToolsEnvVars, goBuiltinTypes, goKeywords, guessPackageNameFromFile, isPositionInComment, isPositionInString, parseFilePrelude, runGodoc } from './util';

Expand Down Expand Up @@ -58,7 +58,7 @@ const exportedMemberRegex = /(const|func|type|var)(\s+\(.*\))?\s+([A-Z]\w*)/;
const gocodeNoSupportForgbMsgKey = 'dontshowNoSupportForgb';

export class GoCompletionItemProvider implements vscode.CompletionItemProvider, vscode.Disposable {
private pkgsList = new Map<string, string>();
private pkgsList = new Map<string, PackageInfo>();
private killMsgShown: boolean = false;
private setGocodeOptions: boolean = true;
private isGoMod: boolean = false;
Expand Down Expand Up @@ -467,8 +467,8 @@ export class GoCompletionItemProvider implements vscode.CompletionItemProvider,
*/
private getPackageImportPath(input: string): string[] {
const matchingPackages: any[] = [];
this.pkgsList.forEach((pkgName: string, pkgPath: string) => {
if (input === pkgName) {
this.pkgsList.forEach((info: PackageInfo, pkgPath: string) => {
if (input === info.name) {
matchingPackages.push(pkgPath);
}
});
Expand Down Expand Up @@ -528,15 +528,16 @@ function getKeywordCompletions(currentWord: string): vscode.CompletionItem[] {
* @param allPkgMap Map of all available packages and their import paths
* @param importedPackages List of imported packages. Used to prune imported packages out of available packages
*/
function getPackageCompletions(document: vscode.TextDocument, currentWord: string, allPkgMap: Map<string, string>, importedPackages: string[] = []): vscode.CompletionItem[] {
function getPackageCompletions(document: vscode.TextDocument, currentWord: string, allPkgMap: Map<string, PackageInfo>, importedPackages: string[] = []): vscode.CompletionItem[] {
const cwd = path.dirname(document.fileName);
const goWorkSpace = getCurrentGoWorkspaceFromGOPATH(getCurrentGoPath(), cwd);
const workSpaceFolder = vscode.workspace.getWorkspaceFolder(document.uri);
const currentPkgRootPath = (workSpaceFolder ? workSpaceFolder.uri.path : cwd).slice(goWorkSpace.length + 1);

const completionItems: any[] = [];

allPkgMap.forEach((pkgName: string, pkgPath: string) => {
allPkgMap.forEach((info: PackageInfo, pkgPath: string) => {
const pkgName = info.name
if (pkgName.startsWith(currentWord) && importedPackages.indexOf(pkgName) === -1) {

const item = new vscode.CompletionItem(pkgName, vscode.CompletionItemKind.Keyword);
Expand Down
2 changes: 1 addition & 1 deletion test/integration/extension.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -552,7 +552,7 @@ It returns the number of bytes written and any write error encountered.

vendorSupportPromise.then(async (vendorSupport: boolean) => {
const gopkgsPromise = getAllPackages(workDir).then(pkgMap => {
const pkgs = Array.from(pkgMap.keys()).filter(p => pkgMap.get(p) !== 'main');
const pkgs = Array.from(pkgMap.keys()).filter(p => pkgMap.get(p).name !== 'main');
if (vendorSupport) {
vendorPkgsFullPath.forEach(pkg => {
assert.equal(pkgs.indexOf(pkg) > -1, true, `Package not found by goPkgs: ${pkg}`);
Expand Down