Rework showing of generic error message #795

mattseddon · 2021-09-08T09:14:34Z

Addresses a big portion of #482

The change here standardises the way that we handle errors with our user facing commands. We now have two different approaches based on whether or not the command relies on the DVC CLI.

Demo CLI based command:

Screen.Recording.2021-09-10.at.10.44.22.am.mov

Demo non-cli command (wired to throw):

Screen.Recording.2021-09-10.at.10.45.43.am.mov

Appreciate this is a big PR and will take some time to review. We had a lot of inconsistent try / catch logic sprinkled throughout the code. I've left lots of comments inline to try and help with the size.

mattseddon · 2021-09-08T09:20:38Z

extension/src/cli/actions.ts


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

-    return showGenericError()
+    throw e


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

mattseddon · 2021-09-08T09:25:10Z

extension/src/commands/external.ts

@@ -60,6 +61,7 @@ export const registerInstrumentedCommand = <T = string | undefined>(
      })
      return res
    } catch (e: unknown) {
+      showGenericError()
      sendTelemetryEventAndThrow(name, e as Error, stopWatch.getElapsedTime())


[F] We probably don't need to throw here anymore as we are putting up a modal to say that something has gone wrong, we might even be able to drop that to a toast if we add a button to show the output channel

mattseddon · 2021-09-09T01:59:31Z

extension/src/commands/internal.ts

+          return res
+        } catch (e: unknown) {
+          showGenericError()
+          this.outputChannel.show()


[F] Want to rework this part now

… rework-default-notifications

mattseddon · 2021-09-09T03:37:09Z

extension/src/commands/internal.ts

+          return res
+        } catch (e: unknown) {
+          this.outputChannel.handleError(e as Error)
+          sendErrorTelemetryEvent(name, e as Error, stopWatch.getElapsedTime())


[F] The difference in these functions is how we handle the error. For cli based commands we just offer to show the output channel for everything else we append the error message to the channel and then offer to show that

mattseddon · 2021-09-09T22:27:00Z

extension/src/commands/internal.ts

+          return await this.runAndSendTelemetry<T>(name, func, arg)
+        } catch (e: unknown) {
+          reportCommandFailed(name)
+          throw e


[C] Test this again, it might show two notifications

mattseddon · 2021-09-10T00:25:33Z

extension/src/vscode/reporting.ts

-  try {
-    window.showInformationMessage((await stdout) || 'Operation successful.')
-  } catch (e) {
-    reportErrorMessage(e)


[F] We now handle this in the same place as the other CLI errors. Made sense to rename the function at the same time.

mattseddon · 2021-09-10T00:26:45Z

extension/src/vscode/reporting.ts

-  }
+export const reportOutput = async (stdout: Promise<string>) => {
+  const output = (await stdout) || 'Operation successful.'
+  window.showInformationMessage(output)


[F] We only need to await the stdout coming through to see if the function throws, we can then send this notification into the void instead of waiting for a response before returning.

mattseddon · 2021-09-10T00:30:32Z

extension/src/commands/internal.ts

+        try {
+          return await this.runAndSendTelemetry<T>(name, func, arg)
+        } catch (e: unknown) {
+          this.outputChannel.offerToShowError()


[F] This is the difference between "CLI" and "non-CLI" commands, if the CLI fails then we will offer to show the output channel to the user, if a command that we have total control over fails then it will fail in the normal VS code way.

mattseddon · 2021-09-10T00:31:11Z

extension/src/commands/external.ts

@@ -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>(


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

mattseddon · 2021-09-10T00:41:38Z

extension/src/commands/external.ts

-import { StopWatch } from '../util/time'
-
-export enum RegisteredCommands {
+export enum RegisteredCliCommands {


[F] The change in this file is to split commands between those that heavily rely on the CLI (generally are just pass through) and those that don't.

mattseddon · 2021-09-10T00:52:41Z

extension/src/experiments/index.ts

@@ -140,7 +140,7 @@ export class Experiments {
      return
    }

-    report(this.internalCommands.executeCommand(commandId, cwd))
+    return reportOutput(this.internalCommands.executeCommand(commandId, cwd))


[F] We are now processing this differently (hence the need for the return) see here for an explanation.

mattseddon · 2021-09-10T00:53:55Z

extension/src/experiments/model/filterBy/tree.test.ts

@@ -5,6 +5,7 @@ import { Operator } from '.'
 import { ExperimentsFilterByTree } from './tree'
 import { Experiments } from '../..'
 import { joinParamOrMetricPath } from '../../paramsAndMetrics/paths'
+import { InternalCommands } from '../../../commands/internal'


[F] Changes in here are a constructor signature change. I had to add InternalCommands into all of the trees to register commands. I did this instead of exposing through the experiments class as that means that we will have the option to break up InternalCommands in the future.

mattseddon · 2021-09-10T01:01:08Z

extension/src/vscode/modal.test.ts

@@ -39,16 +39,3 @@ describe('getWarningResponse', () => {
    expect(mockedShowWarningMessage).toBeCalledTimes(1)
  })
 })
-
-describe('showGenericError', () => {


[F] This was replaced by a new function inside of the output channel class (we want everything to go through there now).

mattseddon · 2021-09-10T01:02:43Z

extension/src/commands/internal.ts

@@ -49,10 +54,6 @@ export class InternalCommands {
    )

    this.registerCommand(AvailableCommands.GET_THEME, () => config.getTheme())
-
-    this.registerCommand(AvailableCommands.SHOW_OUTPUT_CHANNEL, () =>


[F] This was just a placeholder command.

codeclimate · 2021-09-10T03:03:21Z

Code Climate has analyzed commit 0fe2f7b and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 93.2% (85% is the threshold).

This pull request will bring the total coverage in the repository to 96.0% (0.0% change).

View more on Code Climate.

rogermparent

Everything looks fine to me! Took a bit of time to review, but mostly just renames and repeated minor refactors that could be scrolled by- the comments were really helpful on this one!

mattseddon added the product PR that affects product label Sep 8, 2021

mattseddon self-assigned this Sep 8, 2021

mattseddon mentioned this pull request Sep 8, 2021

Add basic rename command to tracked explorer tree (move) #792

Merged

mattseddon changed the base branch from master to add-rename-command September 8, 2021 09:19

mattseddon commented Sep 8, 2021

View reviewed changes

Base automatically changed from add-rename-command to master September 8, 2021 22:46

mattseddon force-pushed the rework-default-notifications branch 2 times, most recently from bd8403e to 649a779 Compare September 8, 2021 22:58

mattseddon added 2 commits September 9, 2021 10:55

add output channel into internal commands

2e0ba8b

deduplicate with build dependencies function

85ef140

mattseddon mentioned this pull request Sep 9, 2021

Add output channel into internal commands #798

Merged

rework how and when we show generic error message

9b2ab84

mattseddon force-pushed the rework-default-notifications branch from 649a779 to 9b2ab84 Compare September 9, 2021 01:09

mattseddon added 4 commits September 9, 2021 11:25

duplicate external registration inside internal commands

5a9bc48

register more commands using internal commands

e9cee45

register remaining commands with internal commands facade

2e7db3d

swap order in function signature for experiment command registration

2f8b70e

mattseddon commented Sep 9, 2021

View reviewed changes

mattseddon added 4 commits September 9, 2021 12:22

show generic error and do not throw on dvc error

6ff2cfe

do not expect an error for commands that will fail because of the cli

9cf6662

split commands between cli and non cli

c7b1ed3

Merge branch 'master' of https://github.com/iterative/vscode-dvc into…

d685db1

… rework-default-notifications

mattseddon commented Sep 9, 2021

View reviewed changes

mattseddon added 4 commits September 9, 2021 13:40

update test wording

2c33cc2

remove dead code

7a61713

only direct user to dvc channel for cli based errors

3f7190d

fix integration test to expect error again

71a869b

mattseddon commented Sep 9, 2021

View reviewed changes

let vs code handle error message for failed non-cli command

1244b9a

do not throw inside report pass the error up

ffefc29

mattseddon commented Sep 10, 2021

View reviewed changes

split commands enums between cli and non cli

8387f44

mattseddon commented Sep 10, 2021

View reviewed changes

mattseddon added 3 commits September 10, 2021 10:55

remove erroneuos brackets

b02bc52

revert to throwing error if change of execution details fails

8f06b10

send the correct error event for stopping an experiment

5a283bc

mattseddon commented Sep 10, 2021

View reviewed changes

mattseddon requested review from rogermparent and shcheklein September 10, 2021 01:04

mattseddon marked this pull request as ready for review September 10, 2021 01:04

mattseddon added 2 commits September 10, 2021 11:38

Merge branch 'master' into rework-default-notifications

c57f470

Merge branch 'master' into rework-default-notifications

0fe2f7b

rogermparent approved these changes Sep 10, 2021

View reviewed changes

mattseddon merged commit 4c2c28b into master Sep 10, 2021

mattseddon deleted the rework-default-notifications branch September 10, 2021 04:32

mattseddon mentioned this pull request Sep 12, 2021

Revisit information / error popups #482

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework showing of generic error message #795

Rework showing of generic error message #795

mattseddon commented Sep 8, 2021 •

edited

Loading

mattseddon Sep 8, 2021

mattseddon Sep 8, 2021

mattseddon Sep 9, 2021

mattseddon Sep 9, 2021

mattseddon Sep 9, 2021

mattseddon Sep 10, 2021

mattseddon Sep 10, 2021

mattseddon Sep 10, 2021

mattseddon Sep 10, 2021

mattseddon Sep 10, 2021

mattseddon Sep 10, 2021

mattseddon Sep 10, 2021

mattseddon Sep 10, 2021

mattseddon Sep 10, 2021

codeclimate bot commented Sep 10, 2021

rogermparent left a comment

Rework showing of generic error message #795

Rework showing of generic error message #795

Conversation

mattseddon commented Sep 8, 2021 • edited Loading

Demo CLI based command:

Demo non-cli command (wired to throw):

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot commented Sep 10, 2021

rogermparent left a comment

Choose a reason for hiding this comment

mattseddon commented Sep 8, 2021 •

edited

Loading