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

Add error message when interpretation fails #770

Merged
merged 2 commits into from
Mar 2, 2021

Conversation

aeisenberg
Copy link
Contributor

One way it can fail is if the SARIF is too large. We explicitly call
out that error because the raw message received from the node runtime
is not very understandable.

Fixes #759

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [n/a] @github/docs-content-dsp has been cc'd in all issues for UI or other user-facing changes made by this pull request.

@aeisenberg aeisenberg requested a review from adityasharad March 2, 2021 19:47
One way it can fail is if the SARIF is too large. We explicitly call
out that error because the raw message received from the node runtime
is not very understandable.
@aeisenberg aeisenberg force-pushed the aeisenberg/interpretation-error branch from f5635c2 to 926856f Compare March 2, 2021 19:48
Copy link
Contributor

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

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

Generally looks good. Minor suggestion on whether we can make the user-facing message shorter and more understandable, so they understand the exact impact.

extensions/ql-vscode/CHANGELOG.md Show resolved Hide resolved
} catch (e) {
const rawMessage = e.stderr || e.message;
const errorMessage = rawMessage.startsWith('Cannot create a string')
? `SARIF too large. ${rawMessage}`
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we log the raw message but make the user-facing one a bit friendlier? Something like "SARIF results are too large to process for the alerts view. Will show raw results instead."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm...I think I can do that.

@aeisenberg aeisenberg enabled auto-merge (rebase) March 2, 2021 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve discoverability of errors that break the alerts view
2 participants