-
Notifications
You must be signed in to change notification settings - Fork 7
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
Improvements for handling CodeChecker version checks #114
Improvements for handling CodeChecker version checks #114
Conversation
* This prevents killing off a version check in the queue, showing a bogus error message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my only remark, otherwise good job!
src/editor/executor.ts
Outdated
@@ -110,7 +110,13 @@ export class ExecutorAlerts { | |||
break; | |||
case ProcessStatus.errored: | |||
this.statusBarItem.text = '$(testing-error-icon) CodeChecker: analysis errored'; | |||
window.showErrorMessage('CodeChecker finished with error - see logs for details'); | |||
|
|||
const shouldShowError = workspace.getConfiguration('codechecker.executor') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't hide Codechecker runtime error notifications in the case when the user ticks the newly introduced option. The settings description says: "Enable notifications when CodeChecker is not found or not supported", but I suspect there can be other errors too. Partial/total suppression of notifications could be introduced, like with a slider or with another tickbox, that says, "Disable every popup originating from the plugin" or something along the way.
If the cases for a failed analysis and other runtime errors are handled elsewhere than this, please consider the above remarks irrelevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the option to Enable CodeChecker-related notifications
, as other notifications being spammed has also been an issue before. It now controls every notification, except when a report cannot be found when trying to jump to it.
I'm planning to show the notification's contents in the sidebar, but that will be added in another PR.
961ff85
to
a5af626
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my remark
a5af626
to
04ddd49
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #111.
CodeChecker version
handling, so a version check process couldn't be prematurely killed, leading to a falseCodeChecker not found
notification. (Should be enough to fix the particular issue showing double notifs.)