diff --git a/extension/src/fileSystem/index.test.ts b/extension/src/fileSystem/index.test.ts index 0cd03c57ed..02a2715f21 100644 --- a/extension/src/fileSystem/index.test.ts +++ b/extension/src/fileSystem/index.test.ts @@ -625,8 +625,8 @@ describe('addPlotToDvcYamlFile', () => { addPlotToDvcYamlFile('/', { template: 'simple', title: 'Simple Plot', - x: { 'data.json': 'epochs' }, - y: { 'data.json': 'accuracy' } + x: { 'data.json': ['epochs'] }, + y: { 'data.json': ['accuracy'] } }) expect(mockedWriteFileSync).toHaveBeenCalledWith( @@ -654,8 +654,38 @@ describe('addPlotToDvcYamlFile', () => { addPlotToDvcYamlFile('/', { template: 'simple', title: 'simple_plot', - x: { 'data.json': 'epochs' }, - y: { 'acc.json': 'accuracy' } + x: { 'data.json': ['epochs'] }, + y: { 'acc.json': ['accuracy'] } + }) + + expect(mockedWriteFileSync).toHaveBeenCalledWith( + '//dvc.yaml', + mockDvcYamlContent + mockPlotYamlContent + ) + }) + + it('should add the new plot with an axis having multiple fields', () => { + const mockDvcYamlContent = mockStagesLines.join('\n') + const mockPlotYamlContent = [ + '', + 'plots:', + ' - simple_plot:', + ' template: simple', + ' x: epochs', + ' y:', + ' data.json:', + ' - accuracy', + ' - epochs', + '' + ].join('\n') + mockedReadFileSync.mockReturnValueOnce(mockDvcYamlContent) + mockedReadFileSync.mockReturnValueOnce(mockDvcYamlContent) + + addPlotToDvcYamlFile('/', { + template: 'simple', + title: 'simple_plot', + x: { 'data.json': ['epochs'] }, + y: { 'data.json': ['accuracy', 'epochs'] } }) expect(mockedWriteFileSync).toHaveBeenCalledWith( @@ -673,8 +703,8 @@ describe('addPlotToDvcYamlFile', () => { addPlotToDvcYamlFile('/', { template: 'simple', title: 'Simple Plot', - x: { 'data.json': 'epochs' }, - y: { 'data.json': 'accuracy' } + x: { 'data.json': ['epochs'] }, + y: { 'data.json': ['accuracy'] } }) mockDvcYamlContent.splice(7, 0, ...mockPlotYamlContent) @@ -696,8 +726,8 @@ describe('addPlotToDvcYamlFile', () => { addPlotToDvcYamlFile('/', { template: 'simple', title: 'Simple Plot', - x: { 'data.json': 'epochs' }, - y: { 'data.json': 'accuracy' } + x: { 'data.json': ['epochs'] }, + y: { 'data.json': ['accuracy'] } }) expect(mockedWriteFileSync).toHaveBeenCalledWith( @@ -734,8 +764,8 @@ describe('addPlotToDvcYamlFile', () => { addPlotToDvcYamlFile('/', { template: 'simple', title: 'simple_plot', - x: { 'data.json': 'epochs' }, - y: { 'data.json': 'accuracy' } + x: { 'data.json': ['epochs'] }, + y: { 'data.json': ['accuracy'] } }) expect(mockedWriteFileSync).toHaveBeenCalledWith( diff --git a/extension/src/fileSystem/index.ts b/extension/src/fileSystem/index.ts index 72edb1ed4f..59965c13a4 100644 --- a/extension/src/fileSystem/index.ts +++ b/extension/src/fileSystem/index.ts @@ -30,7 +30,7 @@ import { processExists } from '../process/execution' import { getFirstWorkspaceFolder } from '../vscode/workspaceFolders' import { DOT_DVC } from '../cli/dvc/constants' import { delay } from '../util/time' -import { PlotConfigData } from '../pipeline/quickPick' +import { PlotConfigData, PlotConfigDataAxis } from '../pipeline/quickPick' export const exists = (path: string): boolean => existsSync(path) @@ -214,18 +214,35 @@ const loadYamlAsDoc = ( } } +const formatPlotYamlObjAxis = (axis: PlotConfigDataAxis) => { + const formattedAxis: { [file: string]: string | string[] } = {} + + for (const [file, fields] of Object.entries(axis)) { + if (fields.length === 1) { + formattedAxis[file] = fields[0] + continue + } + + formattedAxis[file] = fields + } + + return formattedAxis +} + const getPlotYamlObj = (plot: PlotConfigData) => { const { x, y, template, title } = plot const yFiles = Object.keys(y) - const [xFile, xKey] = Object.entries(x)[0] - const oneFileUsed = yFiles.length === 1 && yFiles[0] === xFile + const xFiles = Object.keys(x) + const firstXFile = xFiles[0] + const oneFileUsed = + yFiles.length === 1 && xFiles.length === 1 && yFiles[0] === firstXFile return { [title]: { template, - x: oneFileUsed ? xKey : x, - y + x: oneFileUsed ? x[firstXFile][0] : formatPlotYamlObjAxis(x), + y: formatPlotYamlObjAxis(y) } } } diff --git a/extension/src/pipeline/quickPick.test.ts b/extension/src/pipeline/quickPick.test.ts index 6dd7a726d0..79715e0361 100644 --- a/extension/src/pipeline/quickPick.test.ts +++ b/extension/src/pipeline/quickPick.test.ts @@ -2,11 +2,7 @@ import { QuickPickItemKind } from 'vscode' import { pickPlotConfiguration } from './quickPick' import { getInput } from '../vscode/inputBox' import { pickFiles } from '../vscode/resourcePicker' -import { - quickPickOne, - quickPickValue, - quickPickManyValues -} from '../vscode/quickPick' +import { quickPickOne, quickPickUserOrderedValues } from '../vscode/quickPick' import { getFileExtension, loadDataFiles } from '../fileSystem' import { Title } from '../vscode/title' import { Toast } from '../vscode/toast' @@ -16,8 +12,7 @@ const mockedLoadDataFiles = jest.mocked(loadDataFiles) const mockedGetFileExt = jest.mocked(getFileExtension) const mockedGetInput = jest.mocked(getInput) const mockedQuickPickOne = jest.mocked(quickPickOne) -const mockedQuickPickValue = jest.mocked(quickPickValue) -const mockedQuickPickValues = jest.mocked(quickPickManyValues) +const mockedQuickPickUserOrderedValues = jest.mocked(quickPickUserOrderedValues) const mockedToast = jest.mocked(Toast) const mockedShowError = jest.fn() mockedToast.showError = mockedShowError @@ -264,13 +259,9 @@ describe('pickPlotConfiguration', () => { { data: mockValidData, file: '/file.json' } ]) mockedQuickPickOne.mockResolvedValueOnce('simple') - mockedQuickPickValue.mockResolvedValueOnce({ - file: 'file.json', - key: 'actual' - }) - mockedQuickPickValues.mockResolvedValueOnce([ - { file: 'file.json', key: 'prob' } - ]) + mockedQuickPickUserOrderedValues + .mockImplementationOnce(items => Promise.resolve([items[1].value])) + .mockImplementationOnce(items => Promise.resolve([items[1].value])) mockedGetInput.mockResolvedValueOnce('Simple Plot') const result = await pickPlotConfiguration('/') @@ -290,26 +281,28 @@ describe('pickPlotConfiguration', () => { ], 'Pick a Plot Template' ) - expect(mockedQuickPickValue).toHaveBeenCalledWith( + expect(mockedQuickPickUserOrderedValues).toHaveBeenNthCalledWith( + 1, [ { kind: QuickPickItemKind.Separator, label: 'file.json', value: undefined }, - { label: 'actual', value: { file: 'file.json', key: 'actual' } }, - { label: 'prob', value: { file: 'file.json', key: 'prob' } } + { label: 'actual', value: { field: 'actual', file: 'file.json' } }, + { label: 'prob', value: { field: 'prob', file: 'file.json' } } ], { title: Title.SELECT_PLOT_X_METRIC } ) - expect(mockedQuickPickValues).toHaveBeenCalledWith( + expect(mockedQuickPickUserOrderedValues).toHaveBeenNthCalledWith( + 2, [ { kind: QuickPickItemKind.Separator, label: 'file.json', value: undefined }, - { label: 'prob', value: { file: 'file.json', key: 'prob' } } + { label: 'prob', value: { field: 'prob', file: 'file.json' } } ], { title: Title.SELECT_PLOT_Y_METRIC @@ -322,8 +315,8 @@ describe('pickPlotConfiguration', () => { expect(result).toStrictEqual({ template: 'simple', title: 'Simple Plot', - x: { 'file.json': 'actual' }, - y: { 'file.json': 'prob' } + x: { 'file.json': ['actual'] }, + y: { 'file.json': ['prob'] } }) }) @@ -338,50 +331,48 @@ describe('pickPlotConfiguration', () => { ]) mockedQuickPickOne.mockResolvedValueOnce('simple') mockedGetInput.mockResolvedValueOnce('simple_plot') - mockedQuickPickValue.mockResolvedValueOnce({ - file: 'file.json', - key: 'actual' - }) - mockedQuickPickValues.mockResolvedValueOnce([ - { file: 'file2.json', key: 'prob' } - ]) + mockedQuickPickUserOrderedValues + .mockImplementationOnce(items => Promise.resolve([items[1].value])) + .mockImplementationOnce(items => Promise.resolve([items[4].value])) const result = await pickPlotConfiguration('/') - expect(mockedQuickPickValue).toHaveBeenCalledWith( + expect(mockedQuickPickUserOrderedValues).toHaveBeenNthCalledWith( + 1, [ { kind: QuickPickItemKind.Separator, label: 'file.json', value: undefined }, - { label: 'actual', value: { file: 'file.json', key: 'actual' } }, - { label: 'prob', value: { file: 'file.json', key: 'prob' } }, + { label: 'actual', value: { field: 'actual', file: 'file.json' } }, + { label: 'prob', value: { field: 'prob', file: 'file.json' } }, { kind: QuickPickItemKind.Separator, label: 'file2.json', value: undefined }, - { label: 'actual', value: { file: 'file2.json', key: 'actual' } }, - { label: 'prob', value: { file: 'file2.json', key: 'prob' } } + { label: 'actual', value: { field: 'actual', file: 'file2.json' } }, + { label: 'prob', value: { field: 'prob', file: 'file2.json' } } ], { title: Title.SELECT_PLOT_X_METRIC } ) - expect(mockedQuickPickValues).toHaveBeenCalledWith( + expect(mockedQuickPickUserOrderedValues).toHaveBeenNthCalledWith( + 2, [ { kind: QuickPickItemKind.Separator, label: 'file.json', value: undefined }, - { label: 'prob', value: { file: 'file.json', key: 'prob' } }, + { label: 'prob', value: { field: 'prob', file: 'file.json' } }, { kind: QuickPickItemKind.Separator, label: 'file2.json', value: undefined }, - { label: 'actual', value: { file: 'file2.json', key: 'actual' } }, - { label: 'prob', value: { file: 'file2.json', key: 'prob' } } + { label: 'actual', value: { field: 'actual', file: 'file2.json' } }, + { label: 'prob', value: { field: 'prob', file: 'file2.json' } } ], { title: Title.SELECT_PLOT_Y_METRIC @@ -390,8 +381,8 @@ describe('pickPlotConfiguration', () => { expect(result).toStrictEqual({ template: 'simple', title: 'simple_plot', - x: { 'file.json': 'actual' }, - y: { 'file2.json': 'prob' } + x: { 'file.json': ['actual'] }, + y: { 'file2.json': ['prob'] } }) }) @@ -406,54 +397,121 @@ describe('pickPlotConfiguration', () => { ]) mockedQuickPickOne.mockResolvedValueOnce('simple') mockedGetInput.mockResolvedValueOnce('simple_plot') - mockedQuickPickValue.mockResolvedValueOnce({ - file: 'file.json', - key: 'actual' + mockedQuickPickUserOrderedValues + .mockImplementationOnce(items => Promise.resolve([items[1].value])) + .mockImplementationOnce(items => + Promise.resolve([items[3].value, items[4].value, items[5].value]) + ) + + const result = await pickPlotConfiguration('/') + + expect(mockedQuickPickUserOrderedValues).toHaveBeenNthCalledWith( + 1, + [ + { + kind: QuickPickItemKind.Separator, + label: 'file.json', + value: undefined + }, + { label: 'actual', value: { field: 'actual', file: 'file.json' } }, + { label: 'prob', value: { field: 'prob', file: 'file.json' } }, + { + kind: QuickPickItemKind.Separator, + label: 'file2.json', + value: undefined + }, + { label: 'actual', value: { field: 'actual', file: 'file2.json' } }, + { label: 'prob', value: { field: 'prob', file: 'file2.json' } }, + { label: 'step', value: { field: 'step', file: 'file2.json' } } + ], + { title: Title.SELECT_PLOT_X_METRIC } + ) + expect(mockedQuickPickUserOrderedValues).toHaveBeenNthCalledWith( + 2, + [ + { + kind: QuickPickItemKind.Separator, + label: 'file.json', + value: undefined + }, + { label: 'prob', value: { field: 'prob', file: 'file.json' } }, + { + kind: QuickPickItemKind.Separator, + label: 'file2.json', + value: undefined + }, + { label: 'actual', value: { field: 'actual', file: 'file2.json' } }, + { label: 'prob', value: { field: 'prob', file: 'file2.json' } }, + { label: 'step', value: { field: 'step', file: 'file2.json' } } + ], + { + title: Title.SELECT_PLOT_Y_METRIC + } + ) + expect(result).toStrictEqual({ + template: 'simple', + title: 'simple_plot', + x: { 'file.json': ['actual'] }, + y: { 'file2.json': ['actual', 'prob', 'step'] } }) - mockedQuickPickValues.mockResolvedValueOnce([ - { file: 'file2.json', key: 'prob' }, - { file: 'file2.json', key: 'actual' }, - { file: 'file2.json', key: 'step' } + }) + + it('should let the user pick multiple x fields', async () => { + mockedPickFiles.mockResolvedValueOnce(['/file.json', '/file2.json']) + mockedLoadDataFiles.mockResolvedValueOnce([ + { data: mockValidData, file: '/file.json' }, + { + data: mockValidData, + file: '/file2.json' + } ]) + mockedQuickPickOne.mockResolvedValueOnce('simple') + mockedGetInput.mockResolvedValueOnce('simple_plot') + mockedQuickPickUserOrderedValues + .mockImplementationOnce(items => + Promise.resolve([items[2].value, items[4].value]) + ) + .mockImplementationOnce(items => + Promise.resolve([items[1].value, items[3].value]) + ) const result = await pickPlotConfiguration('/') - expect(mockedQuickPickValue).toHaveBeenCalledWith( + expect(mockedQuickPickUserOrderedValues).toHaveBeenNthCalledWith( + 1, [ { kind: QuickPickItemKind.Separator, label: 'file.json', value: undefined }, - { label: 'actual', value: { file: 'file.json', key: 'actual' } }, - { label: 'prob', value: { file: 'file.json', key: 'prob' } }, + { label: 'actual', value: { field: 'actual', file: 'file.json' } }, + { label: 'prob', value: { field: 'prob', file: 'file.json' } }, { kind: QuickPickItemKind.Separator, label: 'file2.json', value: undefined }, - { label: 'actual', value: { file: 'file2.json', key: 'actual' } }, - { label: 'prob', value: { file: 'file2.json', key: 'prob' } }, - { label: 'step', value: { file: 'file2.json', key: 'step' } } + { label: 'actual', value: { field: 'actual', file: 'file2.json' } }, + { label: 'prob', value: { field: 'prob', file: 'file2.json' } } ], { title: Title.SELECT_PLOT_X_METRIC } ) - expect(mockedQuickPickValues).toHaveBeenCalledWith( + expect(mockedQuickPickUserOrderedValues).toHaveBeenNthCalledWith( + 2, [ { kind: QuickPickItemKind.Separator, label: 'file.json', value: undefined }, - { label: 'prob', value: { file: 'file.json', key: 'prob' } }, + { label: 'actual', value: { field: 'actual', file: 'file.json' } }, { kind: QuickPickItemKind.Separator, label: 'file2.json', value: undefined }, - { label: 'actual', value: { file: 'file2.json', key: 'actual' } }, - { label: 'prob', value: { file: 'file2.json', key: 'prob' } }, - { label: 'step', value: { file: 'file2.json', key: 'step' } } + { label: 'prob', value: { field: 'prob', file: 'file2.json' } } ], { title: Title.SELECT_PLOT_Y_METRIC @@ -462,8 +520,8 @@ describe('pickPlotConfiguration', () => { expect(result).toStrictEqual({ template: 'simple', title: 'simple_plot', - x: { 'file.json': 'actual' }, - y: { 'file2.json': ['prob', 'actual', 'step'] } + x: { 'file.json': ['prob'], 'file2.json': ['actual'] }, + y: { 'file.json': ['actual'], 'file2.json': ['prob'] } }) }) @@ -482,18 +540,53 @@ describe('pickPlotConfiguration', () => { }) it('should return early if the user does not pick a x field', async () => { + mockedPickFiles.mockResolvedValue(['/file.json']) + mockedLoadDataFiles.mockResolvedValue([ + { data: mockValidData, file: 'file.json' } + ]) + mockedQuickPickOne.mockResolvedValue('simple') + mockedGetInput.mockResolvedValue('simple_plot') + mockedQuickPickUserOrderedValues + .mockResolvedValueOnce(undefined) + .mockResolvedValueOnce([]) + + const undefinedResult = await pickPlotConfiguration('/') + + expect(mockedQuickPickUserOrderedValues).toHaveBeenCalledTimes(1) + expect(undefinedResult).toStrictEqual(undefined) + + const noFieldsResult = await pickPlotConfiguration('/') + + expect(mockedQuickPickUserOrderedValues).toHaveBeenCalledTimes(2) + expect(noFieldsResult).toStrictEqual(undefined) + }) + + it('should show a toast if user selects more than one key in a file for an x field', async () => { mockedPickFiles.mockResolvedValueOnce(['/file.json']) mockedLoadDataFiles.mockResolvedValueOnce([ { data: mockValidData, file: 'file.json' } ]) mockedQuickPickOne.mockResolvedValueOnce('simple') mockedGetInput.mockResolvedValueOnce('simple_plot') - mockedQuickPickValue.mockResolvedValueOnce(undefined) + mockedQuickPickUserOrderedValues.mockResolvedValueOnce([ + { + file: 'file.json', + key: 'actual' + }, + { + file: 'file.json', + key: 'prob' + } + ]) const result = await pickPlotConfiguration('/') - expect(mockedQuickPickValue).toHaveBeenCalledTimes(1) + expect(mockedQuickPickUserOrderedValues).toHaveBeenCalledTimes(1) expect(result).toStrictEqual(undefined) + expect(mockedShowError).toHaveBeenCalledTimes(1) + expect(mockedShowError).toHaveBeenCalledWith( + 'file.json cannot have more than one metric selected. See [an example](https://dvc.org/doc/user-guide/project-structure/dvcyaml-files#available-configuration-fields) of a plot with multiple x metrics.' + ) }) it('should return early if the user does not pick a y field', async () => { @@ -503,38 +596,131 @@ describe('pickPlotConfiguration', () => { ]) mockedQuickPickOne.mockResolvedValue('simple') mockedGetInput.mockResolvedValue('simple_plot') - mockedQuickPickValue.mockResolvedValue('actual') - mockedQuickPickValues + mockedQuickPickUserOrderedValues + .mockResolvedValueOnce([ + { + file: 'file.json', + key: 'actual' + } + ]) .mockResolvedValueOnce(undefined) + .mockResolvedValueOnce([ + { + file: 'file.json', + key: 'actual' + } + ]) .mockResolvedValueOnce([]) const undefinedResult = await pickPlotConfiguration('/') - expect(mockedQuickPickValues).toHaveBeenCalledTimes(1) + expect(mockedQuickPickUserOrderedValues).toHaveBeenCalledTimes(2) expect(undefinedResult).toStrictEqual(undefined) - const emptyFieldsResult = await pickPlotConfiguration('/') + const noFieldsResult = await pickPlotConfiguration('/') - expect(mockedQuickPickValues).toHaveBeenCalledTimes(2) - expect(emptyFieldsResult).toStrictEqual(undefined) + expect(mockedQuickPickUserOrderedValues).toHaveBeenCalledTimes(4) + expect(noFieldsResult).toStrictEqual(undefined) }) - it('should return early if the user does not pick a title', async () => { + it('should show a toast if user does not select the same amount of x and y fields (if there are multiple x fields)', async () => { mockedPickFiles.mockResolvedValueOnce(['/file.json']) mockedLoadDataFiles.mockResolvedValueOnce([ - { data: mockValidData, file: 'file.json' } + { data: mockValidData, file: 'file.json' }, + { data: mockValidData, file: 'file2.json' } ]) - mockedQuickPickOne.mockResolvedValueOnce('linear') - mockedQuickPickValue.mockResolvedValueOnce({ - file: 'file.json', - key: 'actual' - }) - mockedQuickPickValues.mockResolvedValueOnce([ + mockedQuickPickOne.mockResolvedValueOnce('simple') + mockedGetInput.mockResolvedValueOnce('simple_plot') + mockedQuickPickUserOrderedValues + .mockResolvedValueOnce([ + { + file: 'file.json', + key: 'actual' + }, + { + file: 'file2.json', + key: 'prob' + } + ]) + .mockResolvedValueOnce([ + { + file: 'file.json', + key: 'prob' + } + ]) + + const result = await pickPlotConfiguration('/') + + expect(mockedQuickPickUserOrderedValues).toHaveBeenCalledTimes(2) + expect(result).toStrictEqual(undefined) + expect(mockedShowError).toHaveBeenCalledTimes(1) + expect(mockedShowError).toHaveBeenCalledWith( + 'Found 2 x metrics and 1 y metric(s). When there are multiple x metrics selected there must be an equal number of y metrics. See [an example](https://dvc.org/doc/user-guide/project-structure/dvcyaml-files#available-configuration-fields) of a plot with multiple x metrics.' + ) + }) + + it('should show a toast if user selects multiple x fields and more than one key in a file for an y field', async () => { + mockedPickFiles.mockResolvedValueOnce(['/file.json']) + mockedLoadDataFiles.mockResolvedValueOnce([ + { data: mockValidData, file: 'file.json' }, { - file: 'file.json', - key: 'prob' + data: mockValidData.map((value, ind) => ({ ...value, step: ind })), + file: 'file2.json' } ]) + mockedQuickPickOne.mockResolvedValueOnce('simple') + mockedGetInput.mockResolvedValueOnce('simple_plot') + mockedQuickPickUserOrderedValues + .mockResolvedValueOnce([ + { + file: 'file.json', + key: 'actual' + }, + { + file: 'file2.json', + key: 'prob' + } + ]) + .mockResolvedValueOnce([ + { + file: 'file2.json', + key: 'actual' + }, + { + file: 'file2.json', + key: 'step' + } + ]) + + const result = await pickPlotConfiguration('/') + + expect(mockedQuickPickUserOrderedValues).toHaveBeenCalledTimes(2) + expect(result).toStrictEqual(undefined) + expect(mockedShowError).toHaveBeenCalledTimes(1) + expect(mockedShowError).toHaveBeenCalledWith( + 'file2.json cannot have more than one metric selected. See [an example](https://dvc.org/doc/user-guide/project-structure/dvcyaml-files#available-configuration-fields) of a plot with multiple x metrics.' + ) + }) + + it('should return early if the user does not pick a title', async () => { + mockedPickFiles.mockResolvedValueOnce(['/file.json']) + mockedLoadDataFiles.mockResolvedValueOnce([ + { data: mockValidData, file: 'file.json' } + ]) + mockedQuickPickOne.mockResolvedValueOnce('linear') + mockedQuickPickUserOrderedValues + .mockResolvedValueOnce([ + { + field: 'actual', + file: 'file.json' + } + ]) + .mockResolvedValueOnce([ + { + field: 'prob', + file: 'file.json' + } + ]) mockedGetInput.mockResolvedValueOnce(undefined) const result = await pickPlotConfiguration('/') diff --git a/extension/src/pipeline/quickPick.ts b/extension/src/pipeline/quickPick.ts index 997a00421d..e62025459d 100644 --- a/extension/src/pipeline/quickPick.ts +++ b/extension/src/pipeline/quickPick.ts @@ -8,26 +8,28 @@ import { isValueTree } from '../cli/dvc/contract' import { getFileExtension, loadDataFiles } from '../fileSystem' -import { - quickPickManyValues, - quickPickOne, - quickPickValue -} from '../vscode/quickPick' +import { quickPickOne, quickPickUserOrderedValues } from '../vscode/quickPick' import { pickFiles } from '../vscode/resourcePicker' import { Title } from '../vscode/title' import { Toast } from '../vscode/toast' import { getInput } from '../vscode/inputBox' +export type PlotConfigDataAxis = { [file: string]: string[] } + export type PlotConfigData = { - x: { [file: string]: string } + x: PlotConfigDataAxis template: string title: string - y: { [file: string]: string[] | string } + y: PlotConfigDataAxis } type UnknownValue = Value | ValueTree type FileFields = { file: string; fields: string[] }[] +type QuickPickFieldValues = { file: string; field: string }[] + +const multipleXMetricsExample = + 'See [an example](https://dvc.org/doc/user-guide/project-structure/dvcyaml-files#available-configuration-fields) of a plot with multiple x metrics.' const pickDataFiles = (): Promise => pickFiles(Title.SELECT_PLOT_DATA, { @@ -35,25 +37,95 @@ const pickDataFiles = (): Promise => }) const formatFieldQuickPickValues = ( - values: { file: string; key: string }[] + values: QuickPickFieldValues | undefined ) => { - const formattedFields: PlotConfigData['y'] = {} + if (!values || values.length === 0) { + return + } + + const formattedFields: PlotConfigDataAxis = {} - for (const { file, key } of values) { + for (const { file, field } of values) { if (!formattedFields[file]) { - formattedFields[file] = key + formattedFields[file] = [field] continue } - const prevFileValue = formattedFields[file] - if (typeof prevFileValue === 'string') { - formattedFields[file] = [prevFileValue] + formattedFields[file].push(field) + } + + return formattedFields +} + +const verifyFilesHaveSingleField = (files: PlotConfigDataAxis) => { + for (const [file, fields] of Object.entries(files)) { + if (fields.length > 1) { + void Toast.showError( + `${file} cannot have more than one metric selected. ${multipleXMetricsExample}` + ) + return } + } + + return files +} + +const verifyXFields = (xValues: QuickPickFieldValues | undefined) => { + const x = formatFieldQuickPickValues(xValues) - ;(formattedFields[file] as string[]).push(key) + if (!x) { + return } - return formattedFields + const doFilesHaveOneField = verifyFilesHaveSingleField(x) + + if (!doFilesHaveOneField) { + return + } + + return x +} + +const verifyYFieldsWithMultiXFields = ( + y: PlotConfigDataAxis, + yValues: QuickPickFieldValues, + xFieldsLength: number +) => { + if (yValues.length !== xFieldsLength) { + void Toast.showError( + `Found ${xFieldsLength} x metrics and ${yValues.length} y metric(s). When there are multiple x metrics selected there must be an equal number of y metrics. ${multipleXMetricsExample}` + ) + return + } + + const doFilesHaveOneField = verifyFilesHaveSingleField(y) + + if (!doFilesHaveOneField) { + return + } + + return y +} + +const verifyYFields = ( + yValues: QuickPickFieldValues | undefined, + xFieldsLength = 0 +) => { + const y = formatFieldQuickPickValues(yValues) + + if (!y) { + return + } + + if (xFieldsLength > 1) { + return verifyYFieldsWithMultiXFields( + y, + yValues as QuickPickFieldValues, + xFieldsLength + ) + } + + return y } const pickFields = async ( @@ -75,34 +147,38 @@ const pickFields = async ( label: file, value: undefined }, - ...fields.map(key => ({ label: key, value: { file, key } })) + ...fields.map(field => ({ label: field, value: { field, file } })) ) } - const xValue = await quickPickValue(items, { + const xValues = (await quickPickUserOrderedValues(items, { title: Title.SELECT_PLOT_X_METRIC - }) + })) as QuickPickFieldValues | undefined - if (!xValue) { + const x = verifyXFields(xValues) + if (!x) { return } - const yValues = (await quickPickManyValues( - items.filter(item => !isEqual(item.value, xValue)), - { title: Title.SELECT_PLOT_Y_METRIC } - )) as { file: string; key: string }[] | undefined + const yValues = (await quickPickUserOrderedValues( + items.filter( + item => item.value === undefined || !xValues?.includes(item.value) + ), + { + title: Title.SELECT_PLOT_Y_METRIC + } + )) as { file: string; field: string }[] | undefined + + const y = verifyYFields(yValues, xValues?.length) - if (!yValues || yValues.length === 0) { + if (!y) { return } return { - fields: { - x: { [xValue.file]: xValue.key }, - y: formatFieldQuickPickValues(yValues) - }, - firstXKey: xValue.key, - firstYKey: yValues[0].key + fields: { x, y }, + firstXKey: (xValues as QuickPickFieldValues)[0].field, + firstYKey: (yValues as QuickPickFieldValues)[0].field } } @@ -224,7 +300,7 @@ const validateMultiFilesData = ( filesData: { data: UnknownValue; file: string }[] ) => { const filesArrLength: Set = new Set() - const keys: FileFields = [] + const fileFields: FileFields = [] for (const { file, data } of filesData) { const metricInfo = getMetricInfoFromValue(data) @@ -234,7 +310,7 @@ const validateMultiFilesData = ( } const { arrLength, fields } = metricInfo - keys.push({ fields, file }) + fileFields.push({ fields, file }) filesArrLength.add(arrLength) } @@ -246,7 +322,7 @@ const validateMultiFilesData = ( return } - return keys + return fileFields } const validateFilesData = async (cwd: string, files: string[]) => { diff --git a/extension/src/test/suite/pipeline/index.test.ts b/extension/src/test/suite/pipeline/index.test.ts index 027fe97012..116164668a 100644 --- a/extension/src/test/suite/pipeline/index.test.ts +++ b/extension/src/test/suite/pipeline/index.test.ts @@ -312,8 +312,8 @@ suite('Pipeline Test Suite', () => { const mockPlotConfig: PipelineQuickPick.PlotConfigData = { template: 'simple', title: 'Great Plot Name', - x: { 'results.json': 'step' }, - y: { 'results.json': 'acc' } + x: { 'results.json': ['step'] }, + y: { 'results.json': ['acc'] } } mockPickPlotConfiguration.onSecondCall().resolves(mockPlotConfig) diff --git a/extension/src/test/suite/util.ts b/extension/src/test/suite/util.ts index bc7fe1ec24..55269f090b 100644 --- a/extension/src/test/suite/util.ts +++ b/extension/src/test/suite/util.ts @@ -85,6 +85,26 @@ export const selectQuickPickItem = async (number: number) => { return commands.executeCommand('workbench.action.acceptSelectedQuickOpenItem') } +const toggleQuickPickItem = async (number: number, itemsLength: number) => { + for (let itemInd = 1; itemInd <= itemsLength; itemInd++) { + await commands.executeCommand('workbench.action.quickOpenSelectNext') + + if (itemInd === number) { + await commands.executeCommand('workbench.action.quickPickManyToggle') + } + } +} + +export const selectMultipleQuickPickItems = async ( + numbers: number[], + itemsLength: number +) => { + for (const number of numbers) { + await toggleQuickPickItem(number, itemsLength) + } + return commands.executeCommand('workbench.action.acceptSelectedQuickOpenItem') +} + export const experimentsUpdatedEvent = (experiments: Experiments) => new Promise(resolve => { experiments.dispose.track(experiments.onDidChangeExperiments(resolve)) diff --git a/extension/src/test/suite/vscode/quickPick.test.ts b/extension/src/test/suite/vscode/quickPick.test.ts index 08d43541dd..85b58e61e9 100644 --- a/extension/src/test/suite/vscode/quickPick.test.ts +++ b/extension/src/test/suite/vscode/quickPick.test.ts @@ -6,9 +6,10 @@ import { Disposable } from '../../../extension' import { QuickPickItemWithValue, quickPickLimitedValues, - quickPickOneOrInput + quickPickOneOrInput, + quickPickUserOrderedValues } from '../../../vscode/quickPick' -import { selectQuickPickItem } from '../util' +import { selectQuickPickItem, selectMultipleQuickPickItems } from '../util' import { Title } from '../../../vscode/title' suite('Quick Pick Test Suite', () => { @@ -140,4 +141,50 @@ suite('Quick Pick Test Suite', () => { expect(quickPick.items, 'all items are shown').to.deep.equal(items) }) }) + + describe('quickPickUserOrderedValues', () => { + it('should return selected values in the order that the user selected', async () => { + const items = [ + { label: 'J', value: 1 }, + { label: 'K', value: 2 }, + { label: 'L', value: 3 }, + { label: 'M', value: 4 }, + { label: 'N', value: 5 } + ] + + const resultPromise = quickPickUserOrderedValues(items, { + title: 'Select some values' as Title + }) + + await selectMultipleQuickPickItems([5, 2, 1], items.length) + + const result = await resultPromise + + expect(result).to.deep.equal([5, 2, 1]) + }) + + it('should return undefined if user cancels the quick pick', async () => { + const quickPick = disposable.track( + window.createQuickPick>() + ) + stub(window, 'createQuickPick').returns(quickPick) + const items = [ + { label: 'J', value: 1 }, + { label: 'K', value: 2 }, + { label: 'L', value: 3 }, + { label: 'M', value: 4 }, + { label: 'N', value: 5 } + ] + + const resultPromise = quickPickUserOrderedValues(items, { + title: 'Select some values' as Title + }) + + quickPick.hide() + + const result = await resultPromise + + expect(result).to.deep.equal(undefined) + }) + }) }) diff --git a/extension/src/vscode/quickPick.ts b/extension/src/vscode/quickPick.ts index 430d5fd739..1b36496675 100644 --- a/extension/src/vscode/quickPick.ts +++ b/extension/src/vscode/quickPick.ts @@ -219,6 +219,64 @@ export const quickPickLimitedValues = ( quickPick.show() }) +const addNewUserOrderedItems = ( + orderedSelection: T[], + newSelection: readonly QuickPickItemWithValue[] +) => { + const itemValues: T[] = [] + for (const item of newSelection) { + if (!orderedSelection.includes(item.value)) { + orderedSelection.push(item.value) + } + itemValues.push(item.value) + } + + return itemValues +} + +const removeMissingUserOrderedItems = ( + orderedSelection: T[], + newSelection: T[] +) => orderedSelection.filter(item => newSelection.includes(item)) + +export const quickPickUserOrderedValues = ( + items: QuickPickItemWithValue[], + options: QuickPickOptions & { title: Title } +): Promise => + new Promise(resolve => { + const quickPick = createQuickPick(items, [], { + ...DEFAULT_OPTIONS, + canSelectMany: true, + ...options + }) + + let orderedSelection: T[] = [] + + quickPick.onDidChangeSelection(selectedItems => { + const selectedItemValues = addNewUserOrderedItems( + orderedSelection, + selectedItems + ) + + orderedSelection = removeMissingUserOrderedItems( + orderedSelection, + selectedItemValues + ) + }) + + quickPick.onDidAccept(() => { + resolve(orderedSelection) + quickPick.dispose() + }) + + quickPick.onDidHide(() => { + resolve(undefined) + quickPick.dispose() + }) + + quickPick.show() + }) + export const quickPickYesOrNo = ( descriptionYes: string, descriptionNo: string,