Merge pull request #3382 from github/robertbrignull/automodel-sort-order

Update method sort order to place auto-model predictions earlier
This commit is contained in:
Robert 2024-02-22 11:00:51 +00:00 коммит произвёл GitHub
Родитель bf35909375 4096ded330
Коммит 7a2688edac
Не найден ключ, соответствующий данной подписи
Идентификатор ключа GPG: B5690EEEBB952194
12 изменённых файлов: 321 добавлений и 61 удалений

Просмотреть файл

@ -63,6 +63,7 @@ export class MethodsUsageDataProvider
mode: Mode,
modeledMethods: Readonly<Record<string, readonly ModeledMethod[]>>,
modifiedMethodSignatures: ReadonlySet<string>,
processedByAutoModelMethods: ReadonlySet<string>,
): Promise<void> {
if (
this.methods !== methods ||
@ -74,7 +75,13 @@ export class MethodsUsageDataProvider
) {
this.methods = methods;
this.sortedTreeItems = createTreeItems(
sortMethodsInGroups(methods, mode),
sortMethodsInGroups(
methods,
modeledMethods,
mode,
modifiedMethodSignatures,
processedByAutoModelMethods,
),
);
this.databaseItem = databaseItem;
this.sourceLocationPrefix =
@ -246,7 +253,13 @@ function urlValueResolvablesAreEqual(
return false;
}
function sortMethodsInGroups(methods: readonly Method[], mode: Mode): Method[] {
function sortMethodsInGroups(
methods: readonly Method[],
modeledMethods: Readonly<Record<string, readonly ModeledMethod[]>>,
mode: Mode,
modifiedMethodSignatures: ReadonlySet<string>,
processedByAutoModelMethods: ReadonlySet<string>,
): Method[] {
const grouped = groupMethods(methods, mode);
const sortedGroupNames = sortGroupNames(grouped);
@ -254,7 +267,12 @@ function sortMethodsInGroups(methods: readonly Method[], mode: Mode): Method[] {
return sortedGroupNames.flatMap((groupName) => {
const group = grouped[groupName];
return sortMethods(group);
return sortMethods(
group,
modeledMethods,
modifiedMethodSignatures,
processedByAutoModelMethods,
);
});
}

Просмотреть файл

@ -39,6 +39,7 @@ export class MethodsUsagePanel extends DisposableObject {
mode: Mode,
modeledMethods: Readonly<Record<string, readonly ModeledMethod[]>>,
modifiedMethodSignatures: ReadonlySet<string>,
processedByAutoModelMethods: ReadonlySet<string>,
): Promise<void> {
await this.dataProvider.setState(
methods,
@ -47,6 +48,7 @@ export class MethodsUsagePanel extends DisposableObject {
mode,
modeledMethods,
modifiedMethodSignatures,
processedByAutoModelMethods,
);
const numOfApis = hideModeledMethods
? methods.filter((api) => !api.supported).length
@ -120,6 +122,7 @@ export class MethodsUsagePanel extends DisposableObject {
activeState.mode,
activeState.modeledMethods,
activeState.modifiedMethodSignatures,
activeState.processedByAutoModelMethods,
);
}
}

Просмотреть файл

@ -19,37 +19,35 @@ export function getCandidates(
modeledMethodsBySignature: Record<string, readonly ModeledMethod[]>,
processedByAutoModelMethods: Set<string>,
): MethodSignature[] {
// Filter out any methods already processed by auto-model
methods = methods.filter(
(m) => !processedByAutoModelMethods.has(m.signature),
);
const candidateMethods = methods.filter((method) => {
// Filter out any methods already processed by auto-model
if (processedByAutoModelMethods.has(method.signature)) {
return false;
}
// Sort the same way as the UI so we send the first ones listed in the UI first
const grouped = groupMethods(methods, mode);
const sortedGroupNames = sortGroupNames(grouped);
const sortedMethods = sortedGroupNames.flatMap((name) =>
sortMethods(grouped[name]),
);
const candidates: MethodSignature[] = [];
for (const method of sortedMethods) {
const modeledMethods: ModeledMethod[] = [
...(modeledMethodsBySignature[method.signature] ?? []),
];
// Anything that is modeled is not a candidate
if (modeledMethods.some((m) => m.type !== "none")) {
continue;
return false;
}
// A method that is supported is modeled outside of the model file, so it is not a candidate.
if (method.supported) {
continue;
return false;
}
// The rest are candidates
candidates.push(method);
}
return candidates;
return true;
});
// Sort the same way as the UI so we send the first ones listed in the UI first
const grouped = groupMethods(candidateMethods, mode);
const sortedGroupNames = sortGroupNames(grouped);
return sortedGroupNames.flatMap((name) =>
// We can safely pass empty sets for `modifiedSignatures` and `processedByAutoModelMethods`
// because we've filtered out all methods that are already modeled or have already been processed by auto-model.
sortMethods(grouped[name], modeledMethodsBySignature, new Set(), new Set()),
);
}

Просмотреть файл

@ -1,4 +1,6 @@
import { canMethodBeModeled } from "../method";
import type { Method } from "../method";
import type { ModeledMethod } from "../modeled-method";
import { Mode } from "./mode";
import { calculateModeledPercentage } from "./modeled-percentage";
@ -27,12 +29,84 @@ export function sortGroupNames(
);
}
export function sortMethods(methods: readonly Method[]): Method[] {
/**
* Primarily sorts methods into the following order:
* - Unsaved positive AutoModel predictions
* - Negative AutoModel predictions
* - Unsaved manual models + unmodeled methods
* - Saved models from this model pack (AutoModel and manual)
* - Methods not modelable in this model pack
*
* Secondary sort order is by number of usages descending, then by method signature ascending.
*/
export function sortMethods(
methods: readonly Method[],
modeledMethodsMap: Record<string, readonly ModeledMethod[]>,
modifiedSignatures: ReadonlySet<string>,
processedByAutoModelMethods: ReadonlySet<string>,
): Method[] {
const sortedMethods = [...methods];
sortedMethods.sort((a, b) => compareMethod(a, b));
sortedMethods.sort((a, b) => {
// First sort by the type of method
const methodAPrimarySortOrdinal = getMethodPrimarySortOrdinal(
a,
modeledMethodsMap[a.signature] ?? [],
modifiedSignatures.has(a.signature),
processedByAutoModelMethods.has(a.signature),
);
const methodBPrimarySortOrdinal = getMethodPrimarySortOrdinal(
b,
modeledMethodsMap[b.signature] ?? [],
modifiedSignatures.has(b.signature),
processedByAutoModelMethods.has(b.signature),
);
if (methodAPrimarySortOrdinal !== methodBPrimarySortOrdinal) {
return methodAPrimarySortOrdinal - methodBPrimarySortOrdinal;
}
// Then sort by number of usages descending
const usageDifference = b.usages.length - a.usages.length;
if (usageDifference !== 0) {
return usageDifference;
}
// Then sort by method signature ascending
return a.signature.localeCompare(b.signature);
});
return sortedMethods;
}
/**
* Assigns numbers to the following classes of methods:
* - Unsaved positive AutoModel predictions => 0
* - Negative AutoModel predictions => 1
* - Unsaved manual models + unmodeled methods => 2
* - Saved models from this model pack (AutoModel and manual) => 3
* - Methods not modelable in this model pack => 4
*/
function getMethodPrimarySortOrdinal(
method: Method,
modeledMethods: readonly ModeledMethod[],
isUnsaved: boolean,
isProcessedByAutoModel: boolean,
): number {
const canBeModeled = canMethodBeModeled(method, modeledMethods, isUnsaved);
const isModeled = modeledMethods.length > 0;
if (canBeModeled) {
if (isModeled && isUnsaved && isProcessedByAutoModel) {
return 0;
} else if (!isModeled && isProcessedByAutoModel) {
return 1;
} else if ((isModeled && isUnsaved) || !isModeled) {
return 2;
} else {
return 3;
}
} else {
return 4;
}
}
function compareGroups(
a: readonly Method[],
aName: string,
@ -69,22 +143,3 @@ function compareGroups(
// Then sort by number of usages descending
return numberOfUsagesB - numberOfUsagesA;
}
function compareMethod(a: Method, b: Method): number {
// Sort first by supported, putting unmodeled methods first.
if (a.supported && !b.supported) {
return 1;
}
if (!a.supported && b.supported) {
return -1;
}
// Then sort by number of usages descending
const usageDifference = b.usages.length - a.usages.length;
if (usageDifference !== 0) {
return usageDifference;
}
// Then sort by method signature ascending
return a.signature.localeCompare(b.signature);
}

Просмотреть файл

@ -48,7 +48,12 @@ export const ModeledMethodDataGrid = ({
] = useMemo(() => {
const methodsWithModelability = [];
let numHiddenMethods = 0;
for (const method of sortMethods(methods)) {
for (const method of sortMethods(
methods,
modeledMethodsMap,
modifiedSignatures,
processedByAutoModelMethods,
)) {
const modeledMethods = modeledMethodsMap[method.signature] ?? [];
const methodIsUnsaved = modifiedSignatures.has(method.signature);
const methodCanBeModeled = canMethodBeModeled(
@ -64,7 +69,13 @@ export const ModeledMethodDataGrid = ({
}
}
return [methodsWithModelability, numHiddenMethods];
}, [hideModeledMethods, methods, modeledMethodsMap, modifiedSignatures]);
}, [
hideModeledMethods,
methods,
modeledMethodsMap,
modifiedSignatures,
processedByAutoModelMethods,
]);
const someMethodsAreVisible = methodsWithModelability.length > 0;

Просмотреть файл

@ -0,0 +1,159 @@
import type { Method } from "../../../../src/model-editor/method";
import type { ModeledMethod } from "../../../../src/model-editor/modeled-method";
import { sortMethods } from "../../../../src/model-editor/shared/sorting";
import {
createMethod,
createUsage,
} from "../../../factories/model-editor/method-factories";
import { createSinkModeledMethod } from "../../../factories/model-editor/modeled-method-factories";
import { shuffle } from "../../../vscode-tests/utils/list-helpers";
describe("sortMethods", () => {
it("uses primary sort order", () => {
const unsavedPositiveAutoModelPrediction = createMethod({
signature: "org.sql2o.Sql2o#unsavedPositiveAutoModelPrediction()",
supported: false,
});
const negativeAutoModelPrediction = createMethod({
signature: "org.sql2o.Sql2o#negativeAutoModelPrediction()",
supported: false,
});
const unsavedManualModel = createMethod({
signature: "org.sql2o.Sql2o#unsavedManualModel()",
supported: false,
});
const unmodeledMethodWithEarlierSignature = createMethod({
signature: "org.sql2o.Sql2o#aaa_unmodeledMethodWithEarlierSignature()",
supported: false,
});
const unmodeledMethodWithLaterSignature = createMethod({
signature: "org.sql2o.Sql2o#zzz_unmodeledMethodWithLaterSignature()",
supported: false,
});
const savedAutoModelPrediction = createMethod({
signature: "org.sql2o.Sql2o#savedAutoModelPrediction()",
supported: false,
});
const savedManualModel = createMethod({
signature: "org.sql2o.Sql2o#savedManualModel()",
supported: false,
});
const supportedMethod = createMethod({
signature: "org.sql2o.Sql2o#supportedMethod()",
supported: true,
});
const methods: Method[] = shuffle([
unsavedPositiveAutoModelPrediction,
negativeAutoModelPrediction,
unsavedManualModel,
unmodeledMethodWithEarlierSignature,
unmodeledMethodWithLaterSignature,
savedAutoModelPrediction,
savedManualModel,
supportedMethod,
]);
const modeledMethodsMap: Record<string, readonly ModeledMethod[]> = {};
modeledMethodsMap[unsavedPositiveAutoModelPrediction.signature] = [
createSinkModeledMethod(),
];
modeledMethodsMap[unsavedManualModel.signature] = [
createSinkModeledMethod(),
];
modeledMethodsMap[savedAutoModelPrediction.signature] = [
createSinkModeledMethod(),
];
modeledMethodsMap[savedManualModel.signature] = [createSinkModeledMethod()];
const modifiedSignatures: Set<string> = new Set([
unsavedPositiveAutoModelPrediction.signature,
unsavedManualModel.signature,
]);
const processedByAutoModelMethods: Set<string> = new Set([
unsavedPositiveAutoModelPrediction.signature,
negativeAutoModelPrediction.signature,
savedAutoModelPrediction.signature,
]);
expect(
sortMethods(
methods,
modeledMethodsMap,
modifiedSignatures,
processedByAutoModelMethods,
),
).toEqual([
unsavedPositiveAutoModelPrediction,
negativeAutoModelPrediction,
unmodeledMethodWithEarlierSignature,
unsavedManualModel,
unmodeledMethodWithLaterSignature,
savedAutoModelPrediction,
savedManualModel,
supportedMethod,
]);
});
it("uses secondary sort order based on usages and signature", () => {
const negativeAutoModelPrediction = createMethod({
signature: "org.sql2o.Sql2o#negativeAutoModelPrediction()",
supported: false,
usages: [],
});
const unmodeledMethodWithTwoUsages = createMethod({
signature: "org.sql2o.Sql2o#unmodeledMethodWithTwoUsages()",
supported: false,
usages: [createUsage(), createUsage()],
});
const unmodeledMethodWithOneUsage = createMethod({
signature: "org.sql2o.Sql2o#unmodeledMethodWithOneUsage()",
supported: false,
usages: [createUsage()],
});
const unmodeledMethodWithEarlierSignature = createMethod({
signature: "org.sql2o.Sql2o#aaa_unmodeledMethodWithEarlierSignature()",
supported: false,
usages: [],
});
const unmodeledMethodWithLaterSignature = createMethod({
signature: "org.sql2o.Sql2o#bbb_unmodeledMethodWithLaterSignature()",
supported: false,
usages: [],
});
const methods: Method[] = shuffle([
negativeAutoModelPrediction,
unmodeledMethodWithTwoUsages,
unmodeledMethodWithOneUsage,
unmodeledMethodWithEarlierSignature,
unmodeledMethodWithLaterSignature,
]);
const modeledMethodsMap: Record<string, readonly ModeledMethod[]> = {};
const modifiedSignatures: Set<string> = new Set([]);
const processedByAutoModelMethods: Set<string> = new Set([
negativeAutoModelPrediction.signature,
]);
expect(
sortMethods(
methods,
modeledMethodsMap,
modifiedSignatures,
processedByAutoModelMethods,
),
).toEqual([
negativeAutoModelPrediction,
unmodeledMethodWithTwoUsages,
unmodeledMethodWithOneUsage,
unmodeledMethodWithEarlierSignature,
unmodeledMethodWithLaterSignature,
]);
});
});

Просмотреть файл

@ -27,6 +27,7 @@ describe("MethodsUsageDataProvider", () => {
const methods: Method[] = [];
const modeledMethods: Record<string, ModeledMethod[]> = {};
const modifiedMethodSignatures: Set<string> = new Set();
const processedByAutoModelMethods: Set<string> = new Set();
const dbItem = mockedObject<DatabaseItem>({
getSourceLocationPrefix: () => "test",
});
@ -39,6 +40,7 @@ describe("MethodsUsageDataProvider", () => {
mode,
modeledMethods,
modifiedMethodSignatures,
processedByAutoModelMethods,
);
const onDidChangeTreeDataListener = jest.fn();
@ -51,6 +53,7 @@ describe("MethodsUsageDataProvider", () => {
mode,
modeledMethods,
modifiedMethodSignatures,
processedByAutoModelMethods,
);
expect(onDidChangeTreeDataListener).not.toHaveBeenCalled();
@ -66,6 +69,7 @@ describe("MethodsUsageDataProvider", () => {
mode,
modeledMethods,
modifiedMethodSignatures,
processedByAutoModelMethods,
);
const onDidChangeTreeDataListener = jest.fn();
@ -78,6 +82,7 @@ describe("MethodsUsageDataProvider", () => {
mode,
modeledMethods,
modifiedMethodSignatures,
processedByAutoModelMethods,
);
expect(onDidChangeTreeDataListener).toHaveBeenCalledTimes(1);
@ -95,6 +100,7 @@ describe("MethodsUsageDataProvider", () => {
mode,
modeledMethods,
modifiedMethodSignatures,
processedByAutoModelMethods,
);
const onDidChangeTreeDataListener = jest.fn();
@ -107,6 +113,7 @@ describe("MethodsUsageDataProvider", () => {
mode,
modeledMethods,
modifiedMethodSignatures,
processedByAutoModelMethods,
);
expect(onDidChangeTreeDataListener).toHaveBeenCalledTimes(1);
@ -120,6 +127,7 @@ describe("MethodsUsageDataProvider", () => {
mode,
modeledMethods,
modifiedMethodSignatures,
processedByAutoModelMethods,
);
const onDidChangeTreeDataListener = jest.fn();
@ -132,6 +140,7 @@ describe("MethodsUsageDataProvider", () => {
mode,
modeledMethods,
modifiedMethodSignatures,
processedByAutoModelMethods,
);
expect(onDidChangeTreeDataListener).toHaveBeenCalledTimes(1);
@ -147,6 +156,7 @@ describe("MethodsUsageDataProvider", () => {
mode,
modeledMethods,
modifiedMethodSignatures,
processedByAutoModelMethods,
);
const onDidChangeTreeDataListener = jest.fn();
@ -159,6 +169,7 @@ describe("MethodsUsageDataProvider", () => {
mode,
modeledMethods2,
modifiedMethodSignatures,
processedByAutoModelMethods,
);
expect(onDidChangeTreeDataListener).toHaveBeenCalledTimes(1);
@ -174,6 +185,7 @@ describe("MethodsUsageDataProvider", () => {
mode,
modeledMethods,
modifiedMethodSignatures,
processedByAutoModelMethods,
);
const onDidChangeTreeDataListener = jest.fn();
@ -186,6 +198,7 @@ describe("MethodsUsageDataProvider", () => {
mode,
modeledMethods,
modifiedMethodSignatures2,
processedByAutoModelMethods,
);
expect(onDidChangeTreeDataListener).toHaveBeenCalledTimes(1);
@ -204,6 +217,7 @@ describe("MethodsUsageDataProvider", () => {
mode,
modeledMethods,
modifiedMethodSignatures,
processedByAutoModelMethods,
);
const onDidChangeTreeDataListener = jest.fn();
@ -216,6 +230,7 @@ describe("MethodsUsageDataProvider", () => {
mode,
modeledMethods,
modifiedMethodSignatures,
processedByAutoModelMethods,
);
expect(onDidChangeTreeDataListener).toHaveBeenCalledTimes(1);
@ -255,6 +270,7 @@ describe("MethodsUsageDataProvider", () => {
createSinkModeledMethod(),
];
const modifiedMethodSignatures: Set<string> = new Set();
const processedByAutoModelMethods: Set<string> = new Set();
const dbItem = mockedObject<DatabaseItem>({
getSourceLocationPrefix: () => "test",
@ -291,6 +307,7 @@ describe("MethodsUsageDataProvider", () => {
mode,
modeledMethods,
modifiedMethodSignatures,
processedByAutoModelMethods,
);
expect(dataProvider.getChildren().length).toEqual(4);
});
@ -304,6 +321,7 @@ describe("MethodsUsageDataProvider", () => {
mode,
modeledMethods,
modifiedMethodSignatures,
processedByAutoModelMethods,
);
expect(dataProvider.getChildren().length).toEqual(3);
});
@ -400,6 +418,7 @@ describe("MethodsUsageDataProvider", () => {
mode,
modeledMethods,
modifiedMethodSignatures,
processedByAutoModelMethods,
);
expect(
dataProvider

Просмотреть файл

@ -28,6 +28,7 @@ describe("MethodsUsagePanel", () => {
const methods: Method[] = [createMethod()];
const modeledMethods: Record<string, ModeledMethod[]> = {};
const modifiedMethodSignatures: Set<string> = new Set();
const processedByAutoModelMethods: Set<string> = new Set();
it("should update the tree view with the correct batch number", async () => {
const mockTreeView = {
@ -50,6 +51,7 @@ describe("MethodsUsagePanel", () => {
mode,
modeledMethods,
modifiedMethodSignatures,
processedByAutoModelMethods,
);
expect(mockTreeView.badge?.value).toBe(1);
@ -65,6 +67,7 @@ describe("MethodsUsagePanel", () => {
const mode = Mode.Application;
const modeledMethods: Record<string, ModeledMethod[]> = {};
const modifiedMethodSignatures: Set<string> = new Set();
const processedByAutoModelMethods: Set<string> = new Set();
const usage = createUsage();
beforeEach(() => {
@ -95,6 +98,7 @@ describe("MethodsUsagePanel", () => {
mode,
modeledMethods,
modifiedMethodSignatures,
processedByAutoModelMethods,
);
await panel.revealItem(method.signature, usage);
@ -122,6 +126,7 @@ describe("MethodsUsagePanel", () => {
mode,
modeledMethods,
modifiedMethodSignatures,
processedByAutoModelMethods,
);
await panel.revealItem(method.signature, usage);

Просмотреть файл

@ -17,7 +17,7 @@ import {
createMockLocalQueryInfo,
createMockQueryWithResults,
} from "../../../factories/query-history/local-query-history-item";
import { shuffleHistoryItems } from "../../utils/query-history-helpers";
import { shuffle } from "../../utils/list-helpers";
import { createMockVariantAnalysisHistoryItem } from "../../../factories/query-history/variant-analysis-history-item";
import type { VariantAnalysisHistoryItem } from "../../../../src/query-history/variant-analysis-history-item";
import { QueryStatus } from "../../../../src/query-history/query-status";
@ -121,10 +121,7 @@ describe("HistoryTreeDataProvider", () => {
variantAnalysisStatus: VariantAnalysisStatus.InProgress,
}),
];
allHistory = shuffleHistoryItems([
...localQueryHistory,
...variantAnalysisHistory,
]);
allHistory = shuffle([...localQueryHistory, ...variantAnalysisHistory]);
labelProvider = new HistoryItemLabelProvider({
format: "",

Просмотреть файл

@ -19,7 +19,7 @@ import {
createMockLocalQueryInfo,
createMockQueryWithResults,
} from "../../../factories/query-history/local-query-history-item";
import { shuffleHistoryItems } from "../../utils/query-history-helpers";
import { shuffle } from "../../utils/list-helpers";
import { createMockVariantAnalysisHistoryItem } from "../../../factories/query-history/variant-analysis-history-item";
import type { VariantAnalysisHistoryItem } from "../../../../src/query-history/variant-analysis-history-item";
import { QueryStatus } from "../../../../src/query-history/query-status";
@ -138,10 +138,7 @@ describe("QueryHistoryManager", () => {
variantAnalysisStatus: VariantAnalysisStatus.InProgress,
}),
];
allHistory = shuffleHistoryItems([
...localQueryHistory,
...variantAnalysisHistory,
]);
allHistory = shuffle([...localQueryHistory, ...variantAnalysisHistory]);
});
afterEach(async () => {

Просмотреть файл

@ -0,0 +1,3 @@
export function shuffle<T>(xs: T[]): T[] {
return xs.sort(() => Math.random() - 0.5);
}

Просмотреть файл

@ -1,5 +0,0 @@
import type { QueryHistoryInfo } from "../../../src/query-history/query-history-info";
export function shuffleHistoryItems(history: QueryHistoryInfo[]) {
return history.sort(() => Math.random() - 0.5);
}