Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display error thrown by Plots Diff in Plots tree #3569

Merged
merged 1 commit into from
Apr 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion extension/src/experiments/columns/tree.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
})
Expand Down Expand Up @@ -498,6 +498,7 @@ describe('ExperimentsColumnsTree', () => {
},
description: '3/4',
iconPath: mockedSelectedCheckbox,
tooltip: undefined,
uri: filename
})
})
Expand Down Expand Up @@ -538,6 +539,7 @@ describe('ExperimentsColumnsTree', () => {
title: 'toggle'
},
iconPath: mockedEmptyCheckbox,
tooltip: undefined,
uri: filename
})
})
Expand Down
16 changes: 11 additions & 5 deletions extension/src/experiments/columns/tree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<WorkspaceExperiments> {
constructor(
Expand All @@ -31,11 +32,16 @@ export class ExperimentsColumnsTree extends BasePathSelectionTree<WorkspaceExper
)
}

protected getBaseTreeItem({
label,
collapsibleState
}: PathSelectionItem & { label: string }) {
return new TreeItem(label, collapsibleState)
public getTreeItem(element: string | PathSelectionItem): TreeItem {
if (isRoot(element)) {
return getRootItem(element)
}

const { label, collapsibleState } = element

const treeItem = new TreeItem(label as string, collapsibleState)

return this.addTreeItemDetails(element, treeItem)
}

protected getRepositoryChildren(dvcRoot: string, path: string) {
Expand Down
8 changes: 2 additions & 6 deletions extension/src/experiments/model/filterBy/tree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {
joinTruthyItems,
sortCollectedArray
} from '../../../util/array'
import { createTreeView, getRootItem } from '../../../tree'
import { createTreeView, getRootItem, isRoot } from '../../../tree'
import { Disposable } from '../../../class/dispose'

export type FilterItem = {
Expand Down Expand Up @@ -66,7 +66,7 @@ export class ExperimentsFilterByTree
}

public getTreeItem(element: string | FilterItem): TreeItem {
if (this.isRoot(element)) {
if (isRoot(element)) {
return getRootItem(element)
}

Expand Down Expand Up @@ -144,10 +144,6 @@ export class ExperimentsFilterByTree
.removeFilter(filter.id)
}

private isRoot(element: string | FilterItem): element is string {
return typeof element === 'string'
}

private getDvcRoots() {
return this.experiments.getDvcRoots()
}
Expand Down
13 changes: 5 additions & 8 deletions extension/src/experiments/model/tree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ import {
DecoratableTreeItemScheme,
getDecoratableTreeItem,
getErrorTooltip,
getRootItem
getRootItem,
isRoot
} from '../../tree'
import { IconName, Resource, ResourceLocator } from '../../resourceLocator'
import {
Expand Down Expand Up @@ -82,7 +83,7 @@ export class ExperimentsTree
}

public getTreeItem(element: string | ExperimentItem): TreeItem {
if (this.isRoot(element)) {
if (isRoot(element)) {
return getRootItem(element)
}

Expand Down Expand Up @@ -121,7 +122,7 @@ export class ExperimentsTree
return this.getRootElements()
}

if (this.isRoot(element)) {
if (isRoot(element)) {
return Promise.resolve(this.getWorkspaceAndCommits(element))
}

Expand Down Expand Up @@ -232,7 +233,7 @@ export class ExperimentsTree
}

private setExpanded(element: string | ExperimentItem, expanded: boolean) {
if (!this.isRoot(element) && element.description) {
if (!isRoot(element) && element.description) {
this.setExperimentExpanded(element.description, expanded)
}
}
Expand Down Expand Up @@ -351,10 +352,6 @@ export class ExperimentsTree
)
}

private isRoot(element: string | ExperimentItem): element is string {
return typeof element === 'string'
}

private getSelectedExperimentItems() {
return [...this.view.selection]
}
Expand Down
6 changes: 4 additions & 2 deletions extension/src/path/selection/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export abstract class PathSelectionModel<

public getTerminalNodeStatuses(parentPath?: string): Status[] {
return (this.getChildren(parentPath) || []).flatMap(element => {
const terminalStatuses = element.hasChildren
const terminalStatuses = (element as T).hasChildren
? this.getTerminalNodeStatuses(element.path)
: [this.status[element.path]]
return [...terminalStatuses]
Expand Down Expand Up @@ -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 })[]
}
86 changes: 38 additions & 48 deletions extension/src/path/selection/tree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string | PathSelectionItem>
implements TreeDataProvider<string | Item>
{
public readonly onDidChangeTreeData: Event<PathSelectionItem | void>

protected readonly workspace: T
protected readonly resourceLocator: ResourceLocator

private readonly view: TreeView<string | PathSelectionItem>
private readonly view: TreeView<string | Item>

private viewed = false
private readonly openEventName: ViewOpenedEventName
Expand All @@ -61,9 +65,7 @@ export abstract class BasePathSelectionTree<

this.onDidChangeTreeData = changeEvent

this.view = this.dispose.track(
createTreeView<PathSelectionItem>(name, this)
)
this.view = this.dispose.track(createTreeView<Item>(name, this))

this.toggleCommand = toggleCommand

Expand All @@ -72,44 +74,17 @@ 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<PathSelectionItem[] | string[]> {
): Promise<Item[] | string[]> {
if (element) {
return Promise.resolve(this.getChildElements(element))
}

return this.getRootElements()
}

protected getIconPath(status?: Status) {
protected getIconPath(status: Status) {
if (status === Status.SELECTED) {
return this.resourceLocator.checkedCheckbox
}
Expand Down Expand Up @@ -155,7 +130,7 @@ export abstract class BasePathSelectionTree<
? TreeItemCollapsibleState.Collapsed
: TreeItemCollapsibleState.None

return {
const pathSelectionItem: PathSelectionItem = {
collapsibleState,
description,
dvcRoot,
Expand All @@ -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() {
Expand Down Expand Up @@ -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)
}

Expand All @@ -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[]
}
33 changes: 28 additions & 5 deletions extension/src/plots/errors/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,29 +5,32 @@ 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()
this.dvcRoot = dvcRoot
}

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) {
Expand All @@ -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<string>()
for (const { name, rev } of this.errors) {
if (selectedRevisions.includes(rev)) {
Expand All @@ -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))
}
}
}
Loading