feat(eslint-plugin): [promise-function-async] Allow any and unknown as return values #553
Conversation
Codecov Report
@@ Coverage Diff @@
## master #553 +/- ##
==========================================
+ Coverage 94.22% 94.26% +0.04%
==========================================
Files 105 105
Lines 4345 4345
Branches 1195 1195
==========================================
+ Hits 4094 4096 +2
+ Misses 146 145 -1
+ Partials 105 104 -1
|
There was a problem hiding this comment.
Thanks for submitting this PR!
A few things
- As suggested in the issue, could you please gate this behind an option which defaults to off (otherwise this is a technically breaking change).
- It would be better to just add a flag which turns off the first check in the
containsTypeByNamefunction - the only place it's used is in this rule.
https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/src/util/types.ts#L14-L42 - As per this comment, because it's a simple if check and prevents another class of invalid reports, could you please also make it so that the rule doesn't check the following 3 cases:
- getters,
- setters,
- constructors
class Foo {
async constructor() { // 'async' modifier cannot appear on a constructor declaration.
await Promise.resolve();
}
async get foo() { // 'async' modifier cannot be used here.
return Promise.resolve();
}
async set foo(value: Promise<void>) { // 'async' modifier cannot be used here.
await Promise.resolve();
}
}
const foo = {
async get foo() { // 'async' modifier cannot be used here.
return Promise.resolve();
},
async set foo(value: Promise<void>) { // 'async' modifier cannot be used here.
await Promise.resolve();
}
}328ea7f to
2ab678a
Compare
2ab678a to
c6fe4da
Compare
bba1fb6 to
3d9cecf
Compare
3d9cecf to
e525fec
Compare
|
@bradzacher i think the PR is ready to be merged |
It's weird to add an option where the default is something nobody would want. This is IMHO just a bug fix. I recommend just doing breaking releases more often. |
|
Breaking releases are a bit of a pain for users and a lot of users miss them because they don't get matched by the default version range the fewer breaking changes released, the easier it is to keep the userbase coherent. In essence I see no reason to major bump unless it's unavoidable, or it's a really bad for a majority of users. |
|
Any chance this could be merged? |
refs #369
i guess it could be improved, but it solves the most obvious issues.