Skip to content

Enable no-explicit-any throughout production code#3353

Merged
robertbrignull merged 7 commits intomainfrom
robertbrignull/eslint_no_any
Feb 14, 2024
Merged

Enable no-explicit-any throughout production code#3353
robertbrignull merged 7 commits intomainfrom
robertbrignull/eslint_no_any

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

This PR contains:

  • the final changes to enable @typescript-eslint/no-explicit-any on all production code (i.e. not tests or build code)
  • fixes for any uses of any where I couldn't find a good solution so I've marked then as ignored

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 13, 2024 15:02
@robertbrignull robertbrignull requested a review from a team as a code owner February 13, 2024 15:02
/**
* A command function is a completely untyped command.
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
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.

I couldn't find a good way around this so ended up just ignoring the eslint error. Admittedly I didn't try super hard, but I didn't want to end up with a solution that was unnecessarily complex either.

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.

I agree that this is the best solution, this is the same error that I ran into yesterday on the CachedOperation, where seemingly ...args: unknown[] isn't well supported.

*
* @param callback The callback to call when the event is triggered.
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
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.

We could rewrite this function to avoid any but I'm hesitant because I don't fully understand what it's doing and it's copied verbatim from another source. We could alternatively rewrite this.

selectedTable: tableName,
},
origResultsPaths: undefined as any, // FIXME: Not used for interpreted, refactor so this is not needed
origResultsPaths: undefined as unknown as ResultsPaths, // FIXME: Not used for interpreted, refactor so this is not needed
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.

This whole situation in ResultsApp.tsx is a bit unfortunate. There are a few arguments where we pass undefined or other "fake" values. I briefly looked at trying to do the refactoring that this comment asks for but I found this area of code very confusing. For now I think doing as unknown as ResultsPaths gets the same effect and isn't any worse than as any. 🤷🏼

Comment on lines +1 to +10
module.exports = {
overrides: [
{
files: ["*"],
rules: {
"@typescript-eslint/no-explicit-any": "off",
},
},
],
};
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.

Our other overrides are in the root .eslintrc.js file itself. Would it make sense to also move this there?

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 good point, you can have multiple overrides for different sets of file paths in the same .eslintrc.js file. Somehow I missed that 🙈 . I'll add them there instead of in separate files.

/**
* A command function is a completely untyped command.
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
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.

I agree that this is the best solution, this is the same error that I ran into yesterday on the CachedOperation, where seemingly ...args: unknown[] isn't well supported.

Comment thread extensions/ql-vscode/test/.eslintrc.js Outdated
Comment on lines +1 to +10
module.exports = {
overrides: [
{
files: ["*"],
rules: {
"@typescript-eslint/no-explicit-any": "off",
},
},
],
};
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.

It would be better to add this to the existing override since doing it here results in two different configs being merged.

@robertbrignull robertbrignull merged commit 9e2b16a into main Feb 14, 2024
@robertbrignull robertbrignull deleted the robertbrignull/eslint_no_any branch February 14, 2024 11:25
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.

2 participants