Skip to content

check for failed tasks#9205

Merged
elahehrashedi merged 7 commits intomainfrom
elrashed/failedtask
Apr 20, 2022
Merged

check for failed tasks#9205
elahehrashedi merged 7 commits intomainfrom
elrashed/failedtask

Conversation

@elahehrashedi
Copy link
Copy Markdown
Contributor

@elahehrashedi elahehrashedi commented Apr 19, 2022

bug fix: #9060
bug fix: #8828

@elahehrashedi elahehrashedi requested a review from a team April 19, 2022 22:11
michelleangela
michelleangela previously approved these changes Apr 19, 2022
Comment thread Extension/src/LanguageServer/cppBuildTaskProvider.ts Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is the real problem that resolve(-1) doesn't exit the function and we immediately send resolve(0) right after this? What happens if you just add a return; after line 444? Maybe the reason this change works is because it now ignores the -1 and 0 returned in the failed build case.

(It's possible you might also need to do something to deal with multiple resolves being called if both the error block and the close block can execute.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In new changes, I kept the result of the promise and checked for when it is -1.

Copy link
Copy Markdown
Member

@bobbrow bobbrow left a comment

Choose a reason for hiding this comment

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

Requesting changes since this PR was approved already, but I'm not confident it's the right way to go.

@michelleangela michelleangela dismissed their stale review April 19, 2022 23:04

Better way to address issue was suggested.

@bobbrow bobbrow dismissed their stale review April 20, 2022 00:21

PR was updated

this.closeEmitter.fire(result);
} catch {
this.closeEmitter.fire(-1);
return -1;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Remove this line.

@elahehrashedi elahehrashedi merged commit 08ab287 into main Apr 20, 2022
@elahehrashedi elahehrashedi deleted the elrashed/failedtask branch May 19, 2022 20:56
@github-actions github-actions Bot locked and limited conversation to collaborators Jun 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

launch occurs (running previous executable file) even when build finishes with errors Debugging shouldn't continue if there's a build error

3 participants