Skip to content

Enable eslint rule to disallow awaiting a non-thenable type#2075

Merged
robertbrignull merged 4 commits intomainfrom
robertbrignull/enable-await-thenable
Feb 15, 2023
Merged

Enable eslint rule to disallow awaiting a non-thenable type#2075
robertbrignull merged 4 commits intomainfrom
robertbrignull/enable-await-thenable

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

Enables https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/await-thenable.md

I spotted that VS Code is giving suggestions for this error in the editor but not in CI. I guess it's not included in @typescript-eslint/recommended. Enabling this eslint rule means we can fix this case and avoid introducing more in the future.

I think this fix in extension.ts is a change of behaviour, because the void operator evaluates the expression and then throws away the result and returns undefined. So previously the chosenAction === installActionName condition was always false.

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 February 14, 2023 17:22
@robertbrignull robertbrignull requested a review from a team as a code owner February 14, 2023 17:22
registerErrorStubs([checkForUpdatesCommand], (command) => async () => {
const installActionName = "Install CodeQL CLI";
const chosenAction = await void showAndLogErrorMessage(
const chosenAction = await showAndLogErrorMessage(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interesting. I guess it was never possible to install the cli from this code path. I never tried this out, though so I don't really know.

Thanks for catching this.

@robertbrignull
Copy link
Copy Markdown
Contributor Author

Oh and the CI found more cases which didn't show up locally for whatever reason. I'll look into them tomorrow.

@robertbrignull
Copy link
Copy Markdown
Contributor Author

I've pushed more linting fixes. I believe all these extra changes are behaviour preserving.

@robertbrignull
Copy link
Copy Markdown
Contributor Author

robertbrignull commented Feb 15, 2023

Ok, third time's the charm. Had to tix another alert that had been introduced on main since I opened this PR.

@robertbrignull robertbrignull merged commit c6ee20b into main Feb 15, 2023
@robertbrignull robertbrignull deleted the robertbrignull/enable-await-thenable branch February 15, 2023 11:26
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.

3 participants