From e4643bad03cc9355e8bf4aeb5c9ce48d6183cce7 Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Tue, 28 Mar 2023 14:30:06 +1100 Subject: [PATCH] Display error thrown by Plots Diff in Plots tree --- .../src/experiments/columns/tree.test.ts | 4 +- extension/src/experiments/columns/tree.ts | 16 ++-- .../src/experiments/model/filterBy/tree.ts | 8 +- extension/src/experiments/model/tree.ts | 13 ++- extension/src/path/selection/model.ts | 6 +- extension/src/path/selection/tree.ts | 86 ++++++++----------- extension/src/plots/errors/model.ts | 33 +++++-- extension/src/plots/index.ts | 3 + extension/src/plots/model/index.ts | 71 +++++++++------ extension/src/plots/paths/model.test.ts | 6 +- extension/src/plots/paths/model.ts | 33 ++++--- extension/src/plots/paths/tree.ts | 37 ++++++-- extension/src/repository/model/collect.ts | 2 +- extension/src/repository/model/error.ts | 16 +--- extension/src/repository/model/index.test.ts | 4 +- extension/src/repository/model/index.ts | 12 +-- extension/src/repository/model/tree.test.ts | 5 +- extension/src/repository/model/tree.ts | 33 +++---- extension/src/test/suite/plots/index.test.ts | 56 ++++++++++++ extension/src/test/suite/plots/util.ts | 5 ++ .../src/test/suite/repository/index.test.ts | 5 +- extension/src/tree/index.ts | 33 +++++++ 22 files changed, 315 insertions(+), 172 deletions(-) diff --git a/extension/src/experiments/columns/tree.test.ts b/extension/src/experiments/columns/tree.test.ts index 268f938f3b..7a2ba55ecf 100644 --- a/extension/src/experiments/columns/tree.test.ts +++ b/extension/src/experiments/columns/tree.test.ts @@ -439,7 +439,7 @@ describe('ExperimentsColumnsTree', () => { it('should return the correct tree item for a repository root', () => { let mockedItem = {} mockedTreeItem.mockImplementationOnce(function (uri, collapsibleState) { - expect(collapsibleState).toStrictEqual(1) + expect(collapsibleState).toStrictEqual(2) mockedItem = { collapsibleState, uri } return mockedItem }) @@ -498,6 +498,7 @@ describe('ExperimentsColumnsTree', () => { }, description: '3/4', iconPath: mockedSelectedCheckbox, + tooltip: undefined, uri: filename }) }) @@ -538,6 +539,7 @@ describe('ExperimentsColumnsTree', () => { title: 'toggle' }, iconPath: mockedEmptyCheckbox, + tooltip: undefined, uri: filename }) }) diff --git a/extension/src/experiments/columns/tree.ts b/extension/src/experiments/columns/tree.ts index c24364b091..2581c7ac3d 100644 --- a/extension/src/experiments/columns/tree.ts +++ b/extension/src/experiments/columns/tree.ts @@ -8,6 +8,7 @@ import { ResourceLocator } from '../../resourceLocator' import { RegisteredCommands } from '../../commands/external' import { EventName } from '../../telemetry/constants' import { InternalCommands } from '../../commands/internal' +import { getRootItem, isRoot } from '../../tree' export class ExperimentsColumnsTree extends BasePathSelectionTree { constructor( @@ -31,11 +32,16 @@ export class ExperimentsColumnsTree extends BasePathSelectionTree { - const terminalStatuses = element.hasChildren + const terminalStatuses = (element as T).hasChildren ? this.getTerminalNodeStatuses(element.path) : [this.status[element.path]] return [...terminalStatuses] @@ -144,7 +144,9 @@ export abstract class PathSelectionModel< abstract getChildren( ...args: unknown[] - ): (T & { descendantStatuses: Status[]; status: Status })[] + ): + | (T & { descendantStatuses: Status[]; status: Status })[] + | { error: string; path: string }[] abstract getTerminalNodes(): (T & { selected: boolean })[] } diff --git a/extension/src/path/selection/tree.ts b/extension/src/path/selection/tree.ts index bad9a396d6..6d932e0a6a 100644 --- a/extension/src/path/selection/tree.ts +++ b/extension/src/path/selection/tree.ts @@ -12,34 +12,38 @@ import { WorkspaceExperiments } from '../../experiments/workspace' import { WorkspacePlots } from '../../plots/workspace' import { Resource, ResourceLocator } from '../../resourceLocator' import { RegisteredCommands } from '../../commands/external' -import { createTreeView } from '../../tree' +import { createTreeView, isRoot } from '../../tree' import { definedAndNonEmpty, sortCollectedArray } from '../../util/array' import { sendViewOpenedTelemetryEvent } from '../../telemetry' import { ViewOpenedEventName } from '../../telemetry/constants' import { Disposable } from '../../class/dispose' +export type ErrorItem = { error: string; path: string } + export type PathSelectionItem = { + collapsibleState: TreeItemCollapsibleState description: string | undefined dvcRoot: string - collapsibleState: TreeItemCollapsibleState + iconPath: Resource | Uri label: string | undefined path: string - iconPath: Resource | Uri tooltip: MarkdownString | undefined } +type Item = ErrorItem | PathSelectionItem + export abstract class BasePathSelectionTree< T extends WorkspaceExperiments | WorkspacePlots > extends Disposable - implements TreeDataProvider + implements TreeDataProvider { public readonly onDidChangeTreeData: Event protected readonly workspace: T protected readonly resourceLocator: ResourceLocator - private readonly view: TreeView + private readonly view: TreeView private viewed = false private readonly openEventName: ViewOpenedEventName @@ -61,9 +65,7 @@ export abstract class BasePathSelectionTree< this.onDidChangeTreeData = changeEvent - this.view = this.dispose.track( - createTreeView(name, this) - ) + this.view = this.dispose.track(createTreeView(name, this)) this.toggleCommand = toggleCommand @@ -72,36 +74,9 @@ export abstract class BasePathSelectionTree< this.updateDescriptionOnChange() } - public getTreeItem(element: string | PathSelectionItem): TreeItem { - if (this.isRoot(element)) { - const resourceUri = Uri.file(element) - return new TreeItem(resourceUri, TreeItemCollapsibleState.Collapsed) - } - - const { dvcRoot, path, description, iconPath, tooltip } = element - - const treeItem = this.getBaseTreeItem(element) - - treeItem.command = { - arguments: [{ dvcRoot, path }], - command: this.toggleCommand, - title: 'toggle' - } - - treeItem.iconPath = iconPath - if (description) { - treeItem.description = description - } - if (tooltip) { - treeItem.tooltip = tooltip - } - - return treeItem - } - public getChildren( element?: string | PathSelectionItem - ): Promise { + ): Promise { if (element) { return Promise.resolve(this.getChildElements(element)) } @@ -109,7 +84,7 @@ export abstract class BasePathSelectionTree< return this.getRootElements() } - protected getIconPath(status?: Status) { + protected getIconPath(status: Status) { if (status === Status.SELECTED) { return this.resourceLocator.checkedCheckbox } @@ -155,7 +130,7 @@ export abstract class BasePathSelectionTree< ? TreeItemCollapsibleState.Collapsed : TreeItemCollapsibleState.None - return { + const pathSelectionItem: PathSelectionItem = { collapsibleState, description, dvcRoot, @@ -164,6 +139,27 @@ export abstract class BasePathSelectionTree< path, tooltip } + + return pathSelectionItem + } + + protected addTreeItemDetails(element: PathSelectionItem, treeItem: TreeItem) { + const { dvcRoot, path, description, iconPath, tooltip } = element + + treeItem.command = { + arguments: [{ dvcRoot, path }], + command: this.toggleCommand, + title: 'toggle' + } + + treeItem.iconPath = iconPath + if (description) { + treeItem.description = description + } + + treeItem.tooltip = tooltip + + return treeItem } private updateDescriptionOnChange() { @@ -196,14 +192,12 @@ export abstract class BasePathSelectionTree< return sortCollectedArray(dvcRoots, (a, b) => a.localeCompare(b)) } - private getChildElements( - element: string | PathSelectionItem - ): PathSelectionItem[] { + private getChildElements(element: string | PathSelectionItem): Item[] { if (!element) { return [] } - if (this.isRoot(element)) { + if (isRoot(element)) { return this.getRepositoryChildren(element, undefined) } @@ -212,16 +206,12 @@ export abstract class BasePathSelectionTree< return this.getRepositoryChildren(dvcRoot, path) } - private isRoot(element: string | PathSelectionItem): element is string { - return typeof element === 'string' - } - - protected abstract getBaseTreeItem(element: PathSelectionItem): TreeItem + abstract getTreeItem(element: string | Item): TreeItem protected abstract getRepositoryChildren( dvcRoot: string, path: string | undefined - ): PathSelectionItem[] + ): Item[] protected abstract getRepositoryStatuses(dvcRoot: string): Status[] } diff --git a/extension/src/plots/errors/model.ts b/extension/src/plots/errors/model.ts index ca2a63937f..9aa588a9ac 100644 --- a/extension/src/plots/errors/model.ts +++ b/extension/src/plots/errors/model.ts @@ -5,13 +5,15 @@ import { collectPathErrorsTable } from './collect' import { Disposable } from '../../class/dispose' -import { PlotError, PlotsOutputOrError } from '../../cli/dvc/contract' +import { DvcError, PlotError, PlotsOutputOrError } from '../../cli/dvc/contract' import { isDvcError } from '../../cli/dvc/reader' +import { getCliErrorLabel } from '../../tree' export class ErrorsModel extends Disposable { private readonly dvcRoot: string private errors: PlotError[] = [] + private cliError: { error: string; path: string } | undefined constructor(dvcRoot: string) { super() @@ -19,15 +21,16 @@ export class ErrorsModel extends Disposable { } public transformAndSet( - data: PlotsOutputOrError, + output: PlotsOutputOrError, revs: string[], cliIdToLabel: { [id: string]: string } ) { - if (isDvcError(data)) { - return + if (isDvcError(output)) { + return this.handleCliError(output) } - this.errors = collectErrors(data, revs, this.errors, cliIdToLabel) + this.errors = collectErrors(output, revs, this.errors, cliIdToLabel) + this.cliError = undefined } public getImageErrors(path: string, revision: string) { @@ -39,6 +42,10 @@ export class ErrorsModel extends Disposable { } public getErrorPaths(selectedRevisions: string[]) { + if (this.cliError) { + return new Set([this.cliError.path]) + } + const acc = new Set() for (const { name, rev } of this.errors) { if (selectedRevisions.includes(rev)) { @@ -47,4 +54,20 @@ export class ErrorsModel extends Disposable { } return acc } + + public hasCliError() { + return !!this.getCliError() + } + + public getCliError() { + return this.cliError + } + + private handleCliError({ error: { msg } }: DvcError) { + this.errors = [] + this.cliError = { + error: msg, + path: join(this.dvcRoot, getCliErrorLabel(msg)) + } + } } diff --git a/extension/src/plots/index.ts b/extension/src/plots/index.ts index 69a757d234..4b7c9b7241 100644 --- a/extension/src/plots/index.ts +++ b/extension/src/plots/index.ts @@ -122,6 +122,9 @@ export class Plots extends BaseRepository { } public getPathStatuses() { + if (this.errors.hasCliError()) { + return [] + } return this.paths.getTerminalNodeStatuses(undefined) } diff --git a/extension/src/plots/model/index.ts b/extension/src/plots/model/index.ts index 9f39b22318..5b2575dc8c 100644 --- a/extension/src/plots/model/index.ts +++ b/extension/src/plots/model/index.ts @@ -13,7 +13,8 @@ import { collectCustomPlots, getCustomPlotId, collectOrderedRevisions, - collectImageUrl + collectImageUrl, + CLIRevisionIdToLabel } from './collect' import { getRevisionFirstThreeColumns } from './util' import { @@ -38,6 +39,7 @@ import { } from '../webview/contract' import { EXPERIMENT_WORKSPACE_ID, + PlotsOutput, PlotsOutputOrError } from '../../cli/dvc/contract' import { Experiments } from '../../experiments' @@ -113,34 +115,17 @@ export class PlotsModel extends ModelWithPersistence { return this.removeStaleData() } - public async transformAndSetPlots(data: PlotsOutputOrError, revs: string[]) { - if (isDvcError(data)) { - return - } - + public async transformAndSetPlots( + output: PlotsOutputOrError, + revs: string[] + ) { const cliIdToLabel = this.getCLIIdToLabel() - const [{ comparisonData, revisionData }, templates, multiSourceVariations] = - await Promise.all([ - collectData(data, cliIdToLabel), - collectTemplates(data), - collectMultiSourceVariations(data, this.multiSourceVariations) - ]) - - this.comparisonData = { - ...this.comparisonData, - ...comparisonData + if (isDvcError(output)) { + this.handleCliError() + } else { + await this.processOutput(output, cliIdToLabel) } - this.revisionData = { - ...this.revisionData, - ...revisionData - } - this.templates = { ...this.templates, ...templates } - this.multiSourceVariations = multiSourceVariations - this.multiSourceEncoding = collectMultiSourceEncoding( - this.multiSourceVariations - ) - this.setComparisonOrder() this.fetchedRevs = new Set(revs.map(rev => cliIdToLabel[rev])) @@ -406,6 +391,40 @@ export class PlotsModel extends ModelWithPersistence { return mapping } + private handleCliError() { + this.comparisonData = {} + this.revisionData = {} + this.templates = {} + this.multiSourceVariations = {} + this.multiSourceEncoding = {} + } + + private async processOutput( + output: PlotsOutput, + cliIdToLabel: CLIRevisionIdToLabel + ) { + const [{ comparisonData, revisionData }, templates, multiSourceVariations] = + await Promise.all([ + collectData(output, cliIdToLabel), + collectTemplates(output), + collectMultiSourceVariations(output, this.multiSourceVariations) + ]) + + this.comparisonData = { + ...this.comparisonData, + ...comparisonData + } + this.revisionData = { + ...this.revisionData, + ...revisionData + } + this.templates = { ...this.templates, ...templates } + this.multiSourceVariations = multiSourceVariations + this.multiSourceEncoding = collectMultiSourceEncoding( + this.multiSourceVariations + ) + } + private cleanupOutdatedCustomPlotsState() { const order = this.getCustomPlotsOrder() const workspaceHoldsUpToDateState = diff --git a/extension/src/plots/paths/model.test.ts b/extension/src/plots/paths/model.test.ts index 48c26a4173..04913d8550 100644 --- a/extension/src/plots/paths/model.test.ts +++ b/extension/src/plots/paths/model.test.ts @@ -22,7 +22,11 @@ describe('PathsModel', () => { ] const buildMockErrorsModel = () => - ({ getPathErrors: () => undefined } as unknown as ErrorsModel) + ({ + getCliError: () => undefined, + getPathErrors: () => undefined, + hasCliError: () => undefined + } as unknown as ErrorsModel) it('should return the expected paths when given the default output fixture', () => { const comparisonType = new Set([PathType.COMPARISON]) diff --git a/extension/src/plots/paths/model.ts b/extension/src/plots/paths/model.ts index d76d415625..5b546e1802 100644 --- a/extension/src/plots/paths/model.ts +++ b/extension/src/plots/paths/model.ts @@ -43,22 +43,20 @@ export class PathsModel extends PathSelectionModel { } public transformAndSet( - data: PlotsOutputOrError, + output: PlotsOutputOrError, revs: string[], cliIdToLabel: { [id: string]: string } ) { - if (isDvcError(data)) { - return + if (isDvcError(output)) { + this.handleCliError() + } else { + const paths = collectPaths(this.data, output, revs, cliIdToLabel) + + this.setNewStatuses(paths) + this.data = paths + this.setTemplateOrder() } - const paths = collectPaths(this.data, data, revs, cliIdToLabel) - - this.setNewStatuses(paths) - - this.data = paths - - this.setTemplateOrder() - this.deferred.resolve() } @@ -79,6 +77,15 @@ export class PathsModel extends PathSelectionModel { path: string | undefined, multiSourceEncoding: MultiSourceEncoding = {} ) { + if (this.errors.hasCliError()) { + return [ + this.errors.getCliError() as { + error: string + path: string + } + ] + } + return this.filterChildren(path) .map(element => ({ ...element, @@ -140,6 +147,10 @@ export class PathsModel extends PathSelectionModel { return this.data.length > 0 } + private handleCliError() { + this.data = [] + } + private getPathsByType( type: PathType, filter = (type: PathType, plotPath: PlotPath) => diff --git a/extension/src/plots/paths/tree.ts b/extension/src/plots/paths/tree.ts index 8b415f0024..5cad81120b 100644 --- a/extension/src/plots/paths/tree.ts +++ b/extension/src/plots/paths/tree.ts @@ -3,6 +3,7 @@ import { TreeItem, TreeItemCollapsibleState } from 'vscode' import { EncodingType, isEncodingElement } from './collect' import { BasePathSelectionTree, + ErrorItem, PathSelectionItem } from '../../path/selection/tree' import { WorkspacePlots } from '../workspace' @@ -10,7 +11,14 @@ import { ResourceLocator } from '../../resourceLocator' import { RegisteredCommands } from '../../commands/external' import { EventName } from '../../telemetry/constants' import { InternalCommands } from '../../commands/internal' -import { DecoratableTreeItemScheme, getDecoratableUri } from '../../tree' +import { + DecoratableTreeItemScheme, + getDecoratableUri, + getCliErrorTreeItem, + getRootItem, + isRoot, + isErrorItem +} from '../../tree' export class PlotsPathsTree extends BasePathSelectionTree { constructor( @@ -34,16 +42,27 @@ export class PlotsPathsTree extends BasePathSelectionTree { ) } - protected getBaseTreeItem({ - dvcRoot, - path, - collapsibleState - }: PathSelectionItem) { + public getTreeItem( + element: string | PathSelectionItem | ErrorItem + ): TreeItem { + if (isRoot(element)) { + return getRootItem(element) + } + + if (isErrorItem(element)) { + const { path, error } = element + return getCliErrorTreeItem(path, error, DecoratableTreeItemScheme.PLOTS) + } + + const { collapsibleState, dvcRoot, path } = element + const resourceUri = getDecoratableUri( join(dvcRoot, path), DecoratableTreeItemScheme.PLOTS ) - return new TreeItem(resourceUri, collapsibleState) + const treeItem = new TreeItem(resourceUri, collapsibleState) + + return this.addTreeItemDetails(element, treeItem) } protected getRepositoryChildren(dvcRoot: string, path: string | undefined) { @@ -51,6 +70,10 @@ export class PlotsPathsTree extends BasePathSelectionTree { .getRepository(dvcRoot) .getChildPaths(path) .map(element => { + if (isErrorItem(element)) { + return element + } + if (isEncodingElement(element)) { const { label, type, value } = element return { diff --git a/extension/src/repository/model/collect.ts b/extension/src/repository/model/collect.ts index 047b77a317..4b74cb445e 100644 --- a/extension/src/repository/model/collect.ts +++ b/extension/src/repository/model/collect.ts @@ -242,7 +242,7 @@ export const collectDataStatus = ( export type PathItem = Resource & { isDirectory: boolean isTracked: boolean - error?: { label: string; msg: string } + error?: string } const transformToAbsTree = ( diff --git a/extension/src/repository/model/error.ts b/extension/src/repository/model/error.ts index dcd956ae45..0e3c41dbca 100644 --- a/extension/src/repository/model/error.ts +++ b/extension/src/repository/model/error.ts @@ -1,29 +1,21 @@ import { PathItem } from './collect' -export type ErrorItem = { error: { label: string; msg: string } } +type ErrorItem = { error: string } export const pathItemHasError = ( maybeErrorItem: T -): maybeErrorItem is T & ErrorItem => - !!(maybeErrorItem?.error?.label && maybeErrorItem?.error?.msg) - -export const getLabel = (msg: string): string => - msg.split('\n')[0].replace(/'|"/g, '') +): maybeErrorItem is T & ErrorItem => !!maybeErrorItem?.error export const createTreeFromError = ( dvcRoot: string, - msg: string, - label: string + msg: string ): Map => new Map([ [ dvcRoot, [ { - error: { - label, - msg - } + error: msg } as PathItem ] ] diff --git a/extension/src/repository/model/index.test.ts b/extension/src/repository/model/index.test.ts index 9a3dc96df9..8f3ccb94a5 100644 --- a/extension/src/repository/model/index.test.ts +++ b/extension/src/repository/model/index.test.ts @@ -253,9 +253,7 @@ describe('RepositoryModel', () => { expect(model.getChildren(dvcDemoPath)).toStrictEqual([]) model.transformAndSet(emptyRepoError) - expect(model.getChildren(dvcDemoPath)).toStrictEqual([ - { error: { label: './dvc.yaml validation failed.', msg } } - ]) + expect(model.getChildren(dvcDemoPath)).toStrictEqual([{ error: msg }]) model.transformAndSet(emptyRepoData) expect(model.getChildren(dvcDemoPath)).toStrictEqual([]) diff --git a/extension/src/repository/model/index.ts b/extension/src/repository/model/index.ts index 9273f35660..206b8d68d1 100644 --- a/extension/src/repository/model/index.ts +++ b/extension/src/repository/model/index.ts @@ -8,7 +8,7 @@ import { DataStatusAccumulator, PathItem } from './collect' -import { createTreeFromError, getLabel } from './error' +import { createTreeFromError } from './error' import { UndecoratedDataStatus } from '../constants' import { SourceControlDataStatus, @@ -24,6 +24,7 @@ import { sameContents } from '../../util/array' import { Data } from '../data' import { isDirectory } from '../../fileSystem' import { DOT_DVC } from '../../cli/dvc/constants' +import { getCliErrorLabel } from '../../tree' export class RepositoryModel extends Disposable { private readonly dvcRoot: string @@ -50,7 +51,7 @@ export class RepositoryModel extends Disposable { public transformAndSet({ dataStatus, hasGitChanges, untracked }: Data) { if (isDvcError(dataStatus)) { - return this.handleError(dataStatus) + return this.handleCliError(dataStatus) } const data = this.collectDataStatus({ @@ -70,16 +71,15 @@ export class RepositoryModel extends Disposable { return this.hasChanges } - private handleError({ error: { msg } }: DvcError) { + private handleCliError({ error: { msg } }: DvcError) { const emptyState = createDataStatusAccumulator() this.hasChanges = true this.tracked = new Set() - const label = getLabel(msg) - this.tree = createTreeFromError(this.dvcRoot, msg, label) + this.tree = createTreeFromError(this.dvcRoot, msg) return { - errorDecorationState: new Set([label]), + errorDecorationState: new Set([getCliErrorLabel(msg)]), scmDecorationState: this.getScmDecorationState(emptyState), sourceControlManagementState: this.getSourceControlManagementState(emptyState) diff --git a/extension/src/repository/model/tree.test.ts b/extension/src/repository/model/tree.test.ts index dded2412ca..b3ffc27224 100644 --- a/extension/src/repository/model/tree.test.ts +++ b/extension/src/repository/model/tree.test.ts @@ -401,10 +401,7 @@ describe('RepositoriesTree', () => { ) const treeItem = trackedTreeView.getTreeItem({ - error: { - label, - msg - } + error: msg } as PathItem) expect(mockedTreeItem).toHaveBeenCalledTimes(1) diff --git a/extension/src/repository/model/tree.ts b/extension/src/repository/model/tree.ts index 7519087dcb..d77d74adc6 100644 --- a/extension/src/repository/model/tree.ts +++ b/extension/src/repository/model/tree.ts @@ -9,7 +9,6 @@ import { Uri } from 'vscode' import { collectSelected, collectTrackedPaths, PathItem } from './collect' -import { ErrorItem, pathItemHasError } from './error' import { Resource } from '../commands' import { WorkspaceRepositories } from '../workspace' import { exists, relativeWithUri } from '../../fileSystem' @@ -42,8 +41,9 @@ import { Disposable } from '../../class/dispose' import { createTreeView, DecoratableTreeItemScheme, - getDecoratableTreeItem, - getErrorTooltip + getCliErrorLabel, + getCliErrorTreeItem, + isErrorItem } from '../../tree' import { getWorkspaceFolders } from '../../vscode/workspaceFolders' import { DOT_DVC } from '../../cli/dvc/constants' @@ -100,8 +100,14 @@ export class RepositoriesTree } public getTreeItem(item: PathItem): TreeItem { - if (pathItemHasError(item)) { - return this.getErrorTreeItem(item) + if (isErrorItem(item)) { + const { error } = item + + return getCliErrorTreeItem( + getCliErrorLabel(error), + error, + DecoratableTreeItemScheme.TRACKED + ) } const { resourceUri, isDirectory } = item @@ -125,23 +131,6 @@ export class RepositoriesTree return treeItem } - private getErrorTreeItem({ error: { msg, label } }: ErrorItem) { - const treeItem = getDecoratableTreeItem( - label, - DecoratableTreeItemScheme.TRACKED - ) - - treeItem.tooltip = getErrorTooltip(msg) - - treeItem.iconPath = new ThemeIcon('blank') - - treeItem.command = { - command: RegisteredCommands.EXTENSION_SHOW_OUTPUT, - title: 'Show DVC Output' - } - return treeItem - } - private reset(): void { this.repositories.treeDataChanged.fire() } diff --git a/extension/src/test/suite/plots/index.test.ts b/extension/src/test/suite/plots/index.test.ts index 6ce7b083f8..cd82afb0e1 100644 --- a/extension/src/test/suite/plots/index.test.ts +++ b/extension/src/test/suite/plots/index.test.ts @@ -46,6 +46,8 @@ import { import { SelectedExperimentWithColor } from '../../../experiments/model' import * as customPlotQuickPickUtil from '../../../plots/model/quickPick' import { CHECKPOINTS_PARAM } from '../../../plots/model/custom' +import { ErrorItem } from '../../../path/selection/tree' +import { isErrorItem } from '../../../tree' suite('Plots Test Suite', () => { const disposable = Disposable.fn() @@ -1074,5 +1076,59 @@ suite('Plots Test Suite', () => { expect(mockSetCustomPlotsOrder).to.not.be.called expect(mockSendTelemetryEvent).to.not.be.called }) + + it('should handle the CLI throwing an error', async () => { + const { data, errorsModel, mockPlotsDiff, plots, plotsModel } = + await buildPlots(disposable, plotsDiffFixture) + + const mockErrorMsg = `'./dvc.yaml' is invalid.\n + While parsing a flow sequence, in line 5, column 9 + 5 │ │ [training/plots/metrics/train/acc.tsv: acc\n + Did not find expected ',' or ']', in line 6, column 44 + 6 │ │ training/plots/metrics/test/acc.tsv: acc]` + + await plots.isReady() + + mockPlotsDiff.resetBehavior() + mockPlotsDiff.resolves({ + error: { msg: mockErrorMsg, type: 'caught error' } + }) + + await data.update() + + const errorItems = plots.getChildPaths(undefined) as ErrorItem[] + + expect( + errorItems, + 'should return a single error item for the plots path tree' + ).to.deep.equal([ + { + error: mockErrorMsg, + path: join(dvcDemoPath, './dvc.yaml is invalid.') + } + ]) + + expect( + errorsModel.getErrorPaths(plotsModel.getSelectedRevisions()), + 'should return the correct path to give the item a DecorationError' + ).to.deep.equal(new Set([errorItems[0].path])) + + mockPlotsDiff.resetBehavior() + mockPlotsDiff.resolves(plotsDiffFixture) + + await data.update() + + const selectionItems = plots.getChildPaths(undefined) as unknown[] + + expect( + selectionItems.filter(item => isErrorItem(item)), + 'should not return any error items after the error is resolved' + ).to.deep.equal([]) + + expect( + errorsModel.getErrorPaths(plotsModel.getSelectedRevisions()), + 'should no long provide decorations to the plots paths tree' + ).to.deep.equal(new Set([])) + }) }) }) diff --git a/extension/src/test/suite/plots/util.ts b/extension/src/test/suite/plots/util.ts index 96c7dd366b..2935fc3d9e 100644 --- a/extension/src/test/suite/plots/util.ts +++ b/extension/src/test/suite/plots/util.ts @@ -25,6 +25,7 @@ import { ExperimentsModel } from '../../../experiments/model' import { Experiment } from '../../../experiments/webview/contract' import { EXPERIMENT_WORKSPACE_ID, PlotsOutput } from '../../../cli/dvc/contract' import { isCheckpointPlot } from '../../../plots/model/custom' +import { ErrorsModel } from '../../../plots/errors/model' export const buildPlots = async ( disposer: Disposer, @@ -97,11 +98,15 @@ export const buildPlots = async ( // eslint-disable-next-line @typescript-eslint/no-explicit-any const pathsModel: PathsModel = (plots as any).paths + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const errorsModel: ErrorsModel = (plots as any).errors + // eslint-disable-next-line @typescript-eslint/no-explicit-any const webviewMessages: WebviewMessages = (plots as any).webviewMessages return { data, + errorsModel, experiments, messageSpy, mockGetModifiedTime, diff --git a/extension/src/test/suite/repository/index.test.ts b/extension/src/test/suite/repository/index.test.ts index 08d4c622f6..7f21a978ea 100644 --- a/extension/src/test/suite/repository/index.test.ts +++ b/extension/src/test/suite/repository/index.test.ts @@ -406,10 +406,7 @@ suite('Repository Test Suite', () => { expect(repository.getChildren(dvcDemoPath)).to.deep.equal([ { - error: { - label: './dvc.yaml validation failed.', - msg - } + error: msg } ]) }) diff --git a/extension/src/tree/index.ts b/extension/src/tree/index.ts index c87a5efd8f..ca0488bb20 100644 --- a/extension/src/tree/index.ts +++ b/extension/src/tree/index.ts @@ -1,5 +1,6 @@ import { MarkdownString, + ThemeIcon, TreeDataProvider, TreeItem, TreeItemCollapsibleState, @@ -8,6 +9,8 @@ import { window } from 'vscode' import { getMarkdownString } from '../vscode/markdownString' +import { RegisteredCommands } from '../commands/external' +import { hasKey } from '../util/object' export enum DecoratableTreeItemScheme { EXPERIMENTS = 'dvc.experiments', @@ -29,9 +32,39 @@ export const getDecoratableTreeItem = ( return new TreeItem(decoratableUri, collapsibleState) } +export type ErrorItem = { error: string } + +export const isErrorItem = ( + maybeErrorItem: unknown +): maybeErrorItem is ErrorItem => hasKey(maybeErrorItem, 'error') + export const getErrorTooltip = (msg: string): MarkdownString => getMarkdownString(`$(error) ${msg}`) +export const getCliErrorLabel = (msg: string): string => + msg.split('\n')[0].replace(/'|"/g, '') + +export const getCliErrorTreeItem = ( + path: string, + msg: string, + decoratableTreeItemScheme: DecoratableTreeItemScheme +) => { + const treeItem = getDecoratableTreeItem(path, decoratableTreeItemScheme) + + treeItem.tooltip = getErrorTooltip(msg) + + treeItem.iconPath = new ThemeIcon('blank') + + treeItem.command = { + command: RegisteredCommands.EXTENSION_SHOW_OUTPUT, + title: 'Show DVC Output' + } + return treeItem +} + +export const isRoot = (element: unknown): element is string => + typeof element === 'string' + export const createTreeView = ( name: string, treeDataProvider: TreeDataProvider,