-
Notifications
You must be signed in to change notification settings - Fork 403
recommended-type-checked lints #1740
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
Conversation
803a1a0 to
69a6d69
Compare
from the 'import/recommended' configuration. not included in the 'import/typescript' config.
not everything is in import/typescript.
it's very difficult to disable TS plugins in the overrides, so omit them from the root and enable them in overrides instead.
As highlighted in the v6 release notes.
these can be re enabled one at a time later.
5230978 to
eaa43ef
Compare
|
There are a bunch of places where there's code like this cosmjs/packages/faucet/src/actions/start.ts Lines 42 to 45 in 553a0de
Which violates https://typescript-eslint.io/rules/no-misused-promises#checksvoidreturn since the async function returns a Promise that a function like setInterval or setTimeout is never going to await on or handle in any way. But I disabled the lint for now. |
webmaster128
left a comment
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.
nice
.eslintrc.js
Outdated
| "@typescript-eslint/prefer-readonly": "off", | ||
| "import/no-unresolved": "off", | ||
| "@typescript-eslint/array-type": ["warn", { default: "array-simple" }], | ||
| "@typescript-eslint/consistent-type-exports": "error", |
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.
We made them all "warn" for better IDE highlighting (which stoped working at some point). The lint CI then rejects all warnings anyways.
| "@typescript-eslint/consistent-type-exports": "error", | |
| "@typescript-eslint/consistent-type-exports": "warn", |
| files: "**/*.spec.ts", | ||
| rules: { | ||
| "@typescript-eslint/no-non-null-assertion": "off", | ||
| "@typescript-eslint/require-await": "off", |
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.
What about ignoring them in-place? How many of those do we have?
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.
For unit tests, there's a lot of boilerplate, and part of the boilerplate is passing async () => ... to the test framework, and it honestly seems very harmless to keep all that boilerplate the same even for tests that don't have an await call in them?
Outside of unit tests... yeah, there's a lot there too. A lot of asynchronous APIs. 15 warnings, but there will be more if you merge #1722
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.
okay sounds good. Yeah no problem having a relaxed linter for test code.
The CI script runs with --max-warnings 0 regardless.
831ba54 to
fcd6c2c
Compare
|
It looks like there's another cosmjs/packages/crypto/src/slip10.ts Line 226 in f4b51d7
But I can't figure out why CI is passing but eslint finds this locally. |
|
https://typescript-eslint.io/rules/no-unnecessary-type-assertion/ But if you actually run Maybe this will be fixed when updating to version 8.0 of the plugin (which supports TypeScript 5.8.3). |
prettify settings changed somehow.
8a4dbee to
b46fd0a
Compare
Here |
|
Probably a local problem I have and that's why it passes on CI! |
webmaster128
left a comment
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.
okay nice. Is this ready to merge?
|
Yep! |
As highlighted in the version 6 announcement, there's always been a separate config with more powerful type-aware lints like
no-floating-promises.https://typescript-eslint.io/blog/announcing-typescript-eslint-v6/
Also enable
import/recommendedsince apparentlyimport/typescriptisn't a superset of it.