Fix Toast messages showing a successful message after failed remote command #5105
Conversation
julieg18
changed the title
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| @@ -21,6 +24,28 @@ export class DvcConfig extends DvcCli { | |||
| return this.executeSafeProcess(cwd, Command.REMOTE, ...args) | |||
| } | |||
|
|
|||
| public remoteAdd(cwd: string, ...args: Args) { | |||
There was a problem hiding this comment.
Similar blocks of code found in 7 locations. Consider refactoring.
| return this.executeDvcProcess(cwd, Command.REMOTE, SubCommand.ADD, ...args) | ||
| } | ||
|
|
||
| public remoteRename(cwd: string, ...args: Args) { |
There was a problem hiding this comment.
Similar blocks of code found in 7 locations. Consider refactoring.
| ) | ||
| } | ||
|
|
||
| public remoteModify(cwd: string, ...args: Args) { |
There was a problem hiding this comment.
Similar blocks of code found in 7 locations. Consider refactoring.
| @@ -21,6 +24,28 @@ export class DvcConfig extends DvcCli { | |||
| return this.executeSafeProcess(cwd, Command.REMOTE, ...args) | |||
There was a problem hiding this comment.
executeSafeProcess catches the error, stopping the external command code from showing the error toast and allowing the Toast.showOutput used in setup/index.ts to get undefined (aka Operation successful).
I've fixed that by adding functions for adding and modifying the remote that don't catch the error thrown.
There was a problem hiding this comment.
any other commands we need to move off safe process?
There was a problem hiding this comment.
I don't think so. Remote list commands are run periodically and remote remove commands are guaranteed to almost always fail since we run them twice in different scopes.
| Flag.PROJECT, | ||
| remote | ||
| ) | ||
| } catch {} |
There was a problem hiding this comment.
I don't think we don't need these try/catch blocks since executeSafeProcess already catches errors.
There was a problem hiding this comment.
is there a test for this?
There was a problem hiding this comment.
I can add a test to ensure remote catches errors.
extension/src/cli/dvc/config.test.ts
Outdated
| }) | ||
|
|
||
| describe('remoteModifyUrl', () => { | ||
| it('should call createProcess with the correct parameters to modify a remote in the config', async () => { |
There was a problem hiding this comment.
[C] might want to state "url" in this description.
|
Code Climate has analyzed commit 0ed1cf2 and detected 3 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 100.0% (85% is the threshold). This pull request will bring the total coverage in the repository to 95.2% (0.0% change). View more on Code Climate. |
Demo
Screen.Recording.2023-12-13.at.11.47.19.AM.mov
Fixes #5095