Skip to content
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

feat(eslint-plugin): [promise-function-async] Allow any and unknown as return values #553

Merged

Conversation

madbence
Copy link
Contributor

refs #369

i guess it could be improved, but it solves the most obvious issues.

@codecov
Copy link

codecov bot commented May 24, 2019

Codecov Report

Merging #553 into master will increase coverage by 0.04%.
The diff coverage is 50%.

@@            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
Impacted Files Coverage Δ
.../eslint-plugin/src/rules/promise-function-async.ts 100% <100%> (ø) ⬆️
packages/eslint-plugin/src/util/types.ts 81.81% <33.33%> (+3.03%) ⬆️

Copy link
Member

@bradzacher bradzacher 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 submitting this PR!
A few things

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();
    }
}

typescript playground
astexplorer

@madbence madbence changed the title [promise-function-async] Allow any and unknown as return values feat(eslint-plugin): [promise-function-async] Allow any and unknown as return values May 27, 2019
@madbence madbence force-pushed the allow-any-unknown-in-func branch 3 times, most recently from bba1fb6 to 3d9cecf Compare May 27, 2019 10:14
@bradzacher bradzacher added the enhancement: plugin rule option New rule option for an existing eslint-plugin rule label May 28, 2019
@madbence
Copy link
Contributor Author

@bradzacher i think the PR is ready to be merged

@bradzacher bradzacher added the 1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready label May 30, 2019
@sindresorhus
Copy link

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'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.

@sindresorhus sindresorhus mentioned this pull request Jun 3, 2019
14 tasks
@bradzacher
Copy link
Member

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 ^, and users don't often run something like yarn outdated. Even then a lot of users shy away from major bumps because it involves reading documentation.

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.
As this is a relatively new rule which also requires type info, I don't think a large portion of the users use it yet (though I don't have stats on that assumption).

@sindresorhus
Copy link

Any chance this could be merged?

@JamesHenry JamesHenry removed the 1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready label Jun 19, 2019
@JamesHenry JamesHenry merged commit 9a387b0 into typescript-eslint:master Jun 19, 2019
@madbence madbence deleted the allow-any-unknown-in-func branch June 20, 2019 10:09
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants