Skip to content

Make use of asError instead of reimplementing#1997

Merged
robertbrignull merged 2 commits intomainfrom
robertbrignull/use_as_error
Jan 23, 2023
Merged

Make use of asError instead of reimplementing#1997
robertbrignull merged 2 commits intomainfrom
robertbrignull/use_as_error

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

This PR updates a few places that were doing their own e instanceOf Error check to instead use asError or getErrorMessage as appropriate.

Also updates the types of the helper methods to be more restrictive, as I believe using unknown instead of any where possible is generally best practice. I haven't gone any further than this with annotating things as unknown to avoid going down too much of a rabbit hole.

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.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@robertbrignull robertbrignull requested a review from a team January 23, 2023 15:52
@robertbrignull robertbrignull requested a review from a team as a code owner January 23, 2023 15:52
Copy link
Copy Markdown
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up!

Comment thread extensions/ql-vscode/src/extension.ts Outdated
Comment on lines +775 to +774
err.message = `Error running query: ${err.message}`;
item.failureReason = err.message;
item.failureReason = `Error running query: ${getErrorMessage(e)}`;
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.

This changes behaviour for the thrown error since the error message now doesn't include Error running query:. The message from the error is probably shown to the user directly since this method is called by a command, so this might change user-facing behaviour as well (depending on if the thrown error is an instance of Error).

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.

Oh your right because asError can just return e instead of a new error, so actually this does edit the existing e which we later throw 🤯

I'll revert this change and leave this file alone.

@robertbrignull robertbrignull merged commit 1381cb6 into main Jan 23, 2023
@robertbrignull robertbrignull deleted the robertbrignull/use_as_error branch January 23, 2023 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants