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

Rework showing of generic error message #795

Merged
merged 23 commits into from
Sep 10, 2021
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
2e0ba8b
add output channel into internal commands
mattseddon Sep 9, 2021
85ef140
deduplicate with build dependencies function
mattseddon Sep 9, 2021
9b2ab84
rework how and when we show generic error message
mattseddon Sep 8, 2021
5a9bc48
duplicate external registration inside internal commands
mattseddon Sep 9, 2021
e9cee45
register more commands using internal commands
mattseddon Sep 9, 2021
2e7db3d
register remaining commands with internal commands facade
mattseddon Sep 9, 2021
2f8b70e
swap order in function signature for experiment command registration
mattseddon Sep 9, 2021
6ff2cfe
show generic error and do not throw on dvc error
mattseddon Sep 9, 2021
9cf6662
do not expect an error for commands that will fail because of the cli
mattseddon Sep 9, 2021
c7b1ed3
split commands between cli and non cli
mattseddon Sep 9, 2021
d685db1
Merge branch 'master' of https://github.com/iterative/vscode-dvc into…
mattseddon Sep 9, 2021
2c33cc2
update test wording
mattseddon Sep 9, 2021
7a61713
remove dead code
mattseddon Sep 9, 2021
3f7190d
only direct user to dvc channel for cli based errors
mattseddon Sep 9, 2021
71a869b
fix integration test to expect error again
mattseddon Sep 9, 2021
1244b9a
let vs code handle error message for failed non-cli command
mattseddon Sep 9, 2021
ffefc29
do not throw inside report pass the error up
mattseddon Sep 10, 2021
8387f44
split commands enums between cli and non cli
mattseddon Sep 10, 2021
b02bc52
remove erroneuos brackets
mattseddon Sep 10, 2021
8f06b10
revert to throwing error if change of execution details fails
mattseddon Sep 10, 2021
5a283bc
send the correct error event for stopping an experiment
mattseddon Sep 10, 2021
c57f470
Merge branch 'master' into rework-default-notifications
mattseddon Sep 10, 2021
0fe2f7b
Merge branch 'master' into rework-default-notifications
mattseddon Sep 10, 2021
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
9 changes: 5 additions & 4 deletions extension/src/cli/actions.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Args, Flag } from './args'
import { Prompt } from './output'
import { getWarningResponse, showGenericError } from '../vscode/modal'
import { MaybeConsoleError } from './error'
import { getWarningResponse } from '../vscode/modal'
import { CommandId, InternalCommands } from '../commands/internal'

const offerToForce = async (
Expand All @@ -27,13 +28,13 @@ export const tryThenMaybeForce = async (
): Promise<string | undefined> => {
try {
return await internalCommands.executeCommand(commandId, ...args)
} catch (e) {
const stderr = e.stderr
} catch (e: unknown) {
const stderr = (e as MaybeConsoleError).stderr

if (stderr?.includes(Prompt.TRY_FORCE)) {
return offerToForce(stderr, internalCommands, commandId, ...args)
}

return showGenericError()
throw e
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[F] We now want to catch these errors in the same place to send an analytics error event

}
}
22 changes: 0 additions & 22 deletions extension/src/commands/external.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,3 @@
import { commands } from 'vscode'
import { Disposable } from '../extension'
import { sendTelemetryEventAndThrow, sendTelemetryEvent } from '../telemetry'
import { StopWatch } from '../util/time'

export enum RegisteredCommands {
EXPERIMENT_APPLY = 'dvc.applyExperiment',
EXPERIMENT_BRANCH = 'dvc.branchExperiment',
Expand Down Expand Up @@ -46,20 +41,3 @@ export enum RegisteredCommands {
TRACKED_EXPLORER_COPY_FILE_PATH = 'dvc.copyFilePath',
TRACKED_EXPLORER_COPY_REL_FILE_PATH = 'dvc.copyRelativeFilePath'
}

export const registerInstrumentedCommand = <T = string | undefined>(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[F] This has been replaced by an InternalCommands method.

name: RegisteredCommands,
func: (arg: T) => unknown
): Disposable =>
commands.registerCommand(name, async arg => {
const stopWatch = new StopWatch()
try {
const res = await func(arg)
sendTelemetryEvent(name, undefined, {
duration: stopWatch.getElapsedTime()
})
return res
} catch (e: unknown) {
sendTelemetryEventAndThrow(name, e as Error, stopWatch.getElapsedTime())
}
})
14 changes: 12 additions & 2 deletions extension/src/commands/internal.test.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,25 @@
import { AvailableCommands, CommandId, InternalCommands } from './internal'
import { ICli } from '../cli'
import { Config } from '../config'
import { OutputChannel } from '../vscode/outputChannel'

const mockedConfig = {
getDefaultProject: jest.fn()
} as unknown as Config

const mockedOutputChannel = {
show: jest.fn()
} as unknown as OutputChannel

beforeEach(() => {
jest.resetAllMocks()
})

describe('InternalCommands', () => {
const internalCommands = new InternalCommands(mockedConfig)
const internalCommands = new InternalCommands(
mockedConfig,
mockedOutputChannel
)

describe('executeCommand', () => {
it('should throw an error if we try to run a non-registered command', () => {
Expand Down Expand Up @@ -39,7 +47,9 @@ describe('InternalCommands', () => {
func: jest.fn()
} as unknown as ICli

expect(() => new InternalCommands(mockedConfig, mockedCli)).toThrow()
expect(
() => new InternalCommands(mockedConfig, mockedOutputChannel, mockedCli)
).toThrow()
})
})
})
47 changes: 43 additions & 4 deletions extension/src/commands/internal.ts
Original file line number Diff line number Diff line change
@@ -1,18 +1,25 @@
import { commands } from 'vscode'
import { Disposable } from '@hediet/std/disposable'
import { RegisteredCommands } from './external'
import { ICli } from '../cli'
import { Args } from '../cli/args'
import { Config } from '../config'
import { quickPickOne } from '../vscode/quickPick'
import { autoRegisteredCommands as CliExecutorCommands } from '../cli/executor'
import { autoRegisteredCommands as CliReaderCommands } from '../cli/reader'
import { autoRegisteredCommands as CliRunnerCommands } from '../cli/runner'
import { Config } from '../config'
import { OutputChannel } from '../vscode/outputChannel'
import { quickPickOne } from '../vscode/quickPick'
import { StopWatch } from '../util/time'
import { sendTelemetryEvent, sendTelemetryEventAndThrow } from '../telemetry'
import { showGenericError } from '../vscode/modal'

type Command = (...args: Args) => unknown | Promise<unknown>

export const AvailableCommands = Object.assign(
{
GET_DEFAULT_OR_PICK_PROJECT: 'getDefaultOrPickProject',
GET_THEME: 'getTheme'
GET_THEME: 'getTheme',
REGISTER_EXTERNAL_COMMAND: 'registerExternalCommand'
} as const,
CliExecutorCommands,
CliReaderCommands,
Expand All @@ -24,9 +31,15 @@ export class InternalCommands {
public dispose = Disposable.fn()

private readonly commands = new Map<string, Command>()
private readonly outputChannel: OutputChannel

constructor(config: Config, ...cliInteractors: ICli[]) {
constructor(
config: Config,
outputChannel: OutputChannel,
...cliInteractors: ICli[]
) {
cliInteractors.forEach(cli => this.autoRegisterCommands(cli))
this.outputChannel = outputChannel

this.registerCommand(
AvailableCommands.GET_DEFAULT_OR_PICK_PROJECT,
Expand Down Expand Up @@ -65,6 +78,32 @@ export class InternalCommands {
this.commands.set(commandId, command)
}

public registerExternalCommand<T = string | undefined>(
name: RegisteredCommands,
func: (arg: T) => unknown
): void {
this.dispose.track(
commands.registerCommand(name, async arg => {
const stopWatch = new StopWatch()
try {
const res = await func(arg)
sendTelemetryEvent(name, undefined, {
duration: stopWatch.getElapsedTime()
})
return res
} catch (e: unknown) {
showGenericError()
this.outputChannel.show()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[F] Want to rework this part now

sendTelemetryEventAndThrow(
name,
e as Error,
stopWatch.getElapsedTime()
)
}
})
)
}

private autoRegisterCommands(cli: ICli) {
cli.autoRegisteredCommands.forEach((commandId: string) => {
if (!this.confirmedId(commandId)) {
Expand Down
126 changes: 70 additions & 56 deletions extension/src/experiments/commands/register.ts
Original file line number Diff line number Diff line change
@@ -1,47 +1,50 @@
import { pickGarbageCollectionFlags } from '../quickPick'
import { Experiments } from '..'
import { AvailableCommands } from '../../commands/internal'
import {
RegisteredCommands,
registerInstrumentedCommand
} from '../../commands/external'
import { AvailableCommands, InternalCommands } from '../../commands/internal'
import { RegisteredCommands } from '../../commands/external'

const registerCommand = (
const registerExperimentCwdCommands = (
experiments: Experiments,
name: RegisteredCommands,
func: (arg?: string) => unknown
): void => {
experiments.dispose.track(registerInstrumentedCommand(name, func))
}

const registerExperimentCwdCommands = (experiments: Experiments): void =>
registerCommand(experiments, RegisteredCommands.QUEUE_EXPERIMENT, () =>
experiments.getCwdThenRun(AvailableCommands.EXPERIMENT_QUEUE)
internalCommands: InternalCommands
): void =>
internalCommands.registerExternalCommand(
RegisteredCommands.QUEUE_EXPERIMENT,
() => experiments.getCwdThenRun(AvailableCommands.EXPERIMENT_QUEUE)
)

const registerExperimentNameCommands = (experiments: Experiments): void => {
registerCommand(experiments, RegisteredCommands.EXPERIMENT_APPLY, () =>
experiments.getExpNameThenRun(AvailableCommands.EXPERIMENT_APPLY)
const registerExperimentNameCommands = (
experiments: Experiments,
internalCommands: InternalCommands
): void => {
internalCommands.registerExternalCommand(
RegisteredCommands.EXPERIMENT_APPLY,
() => experiments.getExpNameThenRun(AvailableCommands.EXPERIMENT_APPLY)
)

registerCommand(experiments, RegisteredCommands.EXPERIMENT_REMOVE, () =>
experiments.getExpNameThenRun(AvailableCommands.EXPERIMENT_REMOVE)
internalCommands.registerExternalCommand(
RegisteredCommands.EXPERIMENT_REMOVE,
() => experiments.getExpNameThenRun(AvailableCommands.EXPERIMENT_REMOVE)
)
}

const registerExperimentInputCommands = (experiments: Experiments): void =>
registerCommand(experiments, RegisteredCommands.EXPERIMENT_BRANCH, () =>
experiments.getExpNameAndInputThenRun(
AvailableCommands.EXPERIMENT_BRANCH,
'Name the new branch'
)
const registerExperimentInputCommands = (
experiments: Experiments,
internalCommands: InternalCommands
): void =>
internalCommands.registerExternalCommand(
RegisteredCommands.EXPERIMENT_BRANCH,
() =>
experiments.getExpNameAndInputThenRun(
AvailableCommands.EXPERIMENT_BRANCH,
'Name the new branch'
)
)

const registerExperimentQuickPickCommands = (
experiments: Experiments
experiments: Experiments,
internalCommands: InternalCommands
): void => {
registerCommand(
experiments,
internalCommands.registerExternalCommand(
RegisteredCommands.EXPERIMENT_GARBAGE_COLLECT,
() =>
experiments.getCwdAndQuickPickThenRun(
Expand All @@ -50,55 +53,66 @@ const registerExperimentQuickPickCommands = (
)
)

registerCommand(
experiments,
internalCommands.registerExternalCommand(
RegisteredCommands.EXPERIMENT_FILTER_ADD,
(dvcRoot?: string) => experiments.addFilter(dvcRoot)
)

registerCommand(
experiments,
internalCommands.registerExternalCommand(
RegisteredCommands.EXPERIMENT_FILTERS_REMOVE,
() => experiments.removeFilters()
)

registerCommand(
experiments,
internalCommands.registerExternalCommand(
RegisteredCommands.EXPERIMENT_SORT_ADD,
(dvcRoot?: string) => experiments.addSort(dvcRoot)
)

registerCommand(experiments, RegisteredCommands.EXPERIMENT_SORTS_REMOVE, () =>
experiments.removeSorts()
internalCommands.registerExternalCommand(
RegisteredCommands.EXPERIMENT_SORTS_REMOVE,
() => experiments.removeSorts()
)
}

const registerExperimentRunCommands = (experiments: Experiments): void => {
registerCommand(experiments, RegisteredCommands.EXPERIMENT_RUN, () =>
experiments.showExperimentsTableThenRun(AvailableCommands.EXPERIMENT_RUN)
const registerExperimentRunCommands = (
experiments: Experiments,
internalCommands: InternalCommands
): void => {
internalCommands.registerExternalCommand(
RegisteredCommands.EXPERIMENT_RUN,
() =>
experiments.showExperimentsTableThenRun(AvailableCommands.EXPERIMENT_RUN)
)

registerCommand(experiments, RegisteredCommands.EXPERIMENT_RUN_RESET, () =>
experiments.showExperimentsTableThenRun(
AvailableCommands.EXPERIMENT_RUN_RESET
)
internalCommands.registerExternalCommand(
RegisteredCommands.EXPERIMENT_RUN_RESET,
() =>
experiments.showExperimentsTableThenRun(
AvailableCommands.EXPERIMENT_RUN_RESET
)
)

registerCommand(experiments, RegisteredCommands.EXPERIMENT_RUN_QUEUED, () =>
experiments.showExperimentsTableThenRun(
AvailableCommands.EXPERIMENT_RUN_QUEUED
)
internalCommands.registerExternalCommand(
RegisteredCommands.EXPERIMENT_RUN_QUEUED,
() =>
experiments.showExperimentsTableThenRun(
AvailableCommands.EXPERIMENT_RUN_QUEUED
)
)

registerCommand(experiments, RegisteredCommands.EXPERIMENT_SHOW, () =>
experiments.showExperimentsTable()
internalCommands.registerExternalCommand(
RegisteredCommands.EXPERIMENT_SHOW,
() => experiments.showExperimentsTable()
)
}

export const registerExperimentCommands = (experiments: Experiments) => {
registerExperimentCwdCommands(experiments)
registerExperimentNameCommands(experiments)
registerExperimentInputCommands(experiments)
registerExperimentQuickPickCommands(experiments)
registerExperimentRunCommands(experiments)
export const registerExperimentCommands = (
experiments: Experiments,
internalCommands: InternalCommands
) => {
registerExperimentCwdCommands(experiments, internalCommands)
registerExperimentNameCommands(experiments, internalCommands)
registerExperimentInputCommands(experiments, internalCommands)
registerExperimentQuickPickCommands(experiments, internalCommands)
registerExperimentRunCommands(experiments, internalCommands)
}
12 changes: 9 additions & 3 deletions extension/src/experiments/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
} from '../commands/internal'
import { getInput } from '../vscode/inputBox'
import { buildMockMemento } from '../test/util'
import { OutputChannel } from '../vscode/outputChannel'

const mockedShowWebview = jest.fn()
const mockedDisposable = mocked(Disposable)
Expand Down Expand Up @@ -39,9 +40,14 @@ beforeEach(() => {
})

describe('Experiments', () => {
const mockedInternalCommands = new InternalCommands({
getDefaultProject: mockedGetDefaultProject
} as unknown as Config)
const mockedInternalCommands = new InternalCommands(
{
getDefaultProject: mockedGetDefaultProject
} as unknown as Config,
{
show: jest.fn()
} as unknown as OutputChannel
)

const mockedCommandId = 'mockedExpFunc' as CommandId
mockedInternalCommands.registerCommand(mockedCommandId, (...args) =>
Expand Down
Loading