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

Fix: added async in allow method in no-empty-function (fixes #12768) #13036

Merged
merged 8 commits into from Mar 20, 2020

Conversation

anikethsaha
Copy link
Member

@anikethsaha anikethsaha commented Mar 12, 2020

Prerequisites checklist

  • I have read the contributing guidelines.
  • The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[X] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Added asyncFunctions and asyncMethods to ALLOW_OPTIONS for no-empty-function rule

Is there anything you'd like reviewers to focus on?

fixes #12768

Also, I am bit confused about how to write tests for valid code as for this rule, gives test cases are converted into valid and invalid

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Mar 12, 2020
Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

Is there anything in particular we can help clarify for you in regards to writing tests?

The test setup is a bit confusing with use of Array.prototype.reduce(). If it's helpful to refactor this to make it easier to understand what's going on, I think that would be fine.

Regarding how tests work, have you looked at the documentation for the RuleTester API?

@kaicataldo kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Mar 12, 2020
@anikethsaha
Copy link
Member Author

The test setup is a bit confusing with use of Array.prototype.reduce(). If it's helpful to refactor this to make it easier to understand what's going on, I think that would be fine.

indeed this is a bit confusing.
I tried adding async code in the initialValue for the reduce method along with the current one
here

I added this

image

but got parsing error

image

@yeonjuan
Copy link
Member

@anikethsaha Thanks for working on it 👍

but got parsing error

You should change the parserOptions with ecmaVersion >= 8 ( 8, 9, ..)

@anikethsaha
Copy link
Member Author

thanks a lot @yeonjuan ! it worked like a charm.
(I thought async are supported in es2015 !)

@kaicataldo I added two test cases. Just do let me know if you need to add some more!

Copy link
Member

@yeonjuan yeonjuan 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 contributing!
Would you mind updating the documentation also? - no-empty-function

tests/lib/rules/no-empty-function.js Outdated Show resolved Hide resolved
@yeonjuan
Copy link
Member

@anikethsaha
Thanks for updating the documentation.
Maybe I didn't describe well at the comment before. sorry 😂

what I understood from this test setup is that these cases are invalid cases and these will be converted into valid and invalid cases. right !

What I meant was that the newly added tests should be in the array at the third parameter of ruleTester.run(), not in the second parameter of Array.prototype.reduce().

Because the test cases at the second parameter of Array.prototype.reduce() are not converted into invalid and valid cases.

  • the cases newly added are not passed in the second parameter(item) of toValidInvalid(patterns, item)

The current test setup seems to like this:

ruleTester.run("no-empty-function", rule, [
   /* 
    * These elements are the test cases that need to be tested with each option and various invalid, valid form.
    * So these elements are converted into other forms by 'toValidInvalid()'
    */
    ,
]).reduce(toValidInvalid, {
  valid: [
    /*
     * These are always valid. So don't need to be converted  
     * That is the reason why "var foo = () => 0;" is here because it is not an empty function.
     */
    {
        code: "var foo = () => 0;", // it has return value '0'. always valid
        parserOptions: { ecmaVersion: 6 }
     }
   ] 

}));

@anikethsaha
Copy link
Member Author

@yeonjuan yes, the second parameter of reduce should contain valid tests and I kept these two cases in valid array with an option to make them valid (allow). which is ok but its not testing all the cases. it makes sense to put them in the 3rd parameter of ruleTester.run to test all cases of these.

I will change this 👍

@yeonjuan
Copy link
Member

. If the pull request addresses an issue, then the issue number should be mentioned at the end.

And it would be nice if we mentioned the issue number in the PR title.

@anikethsaha anikethsaha changed the title Fix: added async in allow method in no-empty-function Fix: added async in allow method in no-empty-function (fixes #12768) Mar 16, 2020
@anikethsaha
Copy link
Member Author

@yeonjuan done 👍

tests/lib/rules/no-empty-function.js Outdated Show resolved Hide resolved
docs/rules/no-empty-function.md Outdated Show resolved Hide resolved
docs/rules/no-empty-function.md Outdated Show resolved Hide resolved
@anikethsaha
Copy link
Member Author

cc @kaicataldo @yeonjuan I added more tests and did the docs changes as request.

Do let me know if anything else needs to be changed!

Copy link
Member

@yeonjuan yeonjuan 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 changing it 👍
I left suggestions for minor typos.

docs/rules/no-empty-function.md Outdated Show resolved Hide resolved
docs/rules/no-empty-function.md Outdated Show resolved Hide resolved
docs/rules/no-empty-function.md Outdated Show resolved Hide resolved
anikethsaha and others added 3 commits March 17, 2020 16:54
Co-Authored-By: YeonJuan <yeonjuan93@naver.com>
Co-Authored-By: YeonJuan <yeonjuan93@naver.com>
Co-Authored-By: YeonJuan <yeonjuan93@naver.com>
@anikethsaha
Copy link
Member Author

cc @yeonjuan done

Copy link
Member

@yeonjuan yeonjuan left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

This LGTM, thank you!

docs/rules/no-empty-function.md Outdated Show resolved Hide resolved
docs/rules/no-empty-function.md Outdated Show resolved Hide resolved
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@kaicataldo kaicataldo merged commit 2260611 into eslint:master Mar 20, 2020
@kaicataldo
Copy link
Member

Thanks for contributing to ESLint!

@anikethsaha anikethsaha deleted the fixes-12768 branch March 20, 2020 07:16
anikethsaha added a commit to anikethsaha/eslint that referenced this pull request Mar 23, 2020
…2768) (eslint#13036)

* Fix: added async in allow method in no-empty-function

* Chore: added tests for async* allow

* Docs: updated docs for async method and funct

* Chore: more tests and test fixes and docs refactore

* Update docs/rules/no-empty-function.md

Co-Authored-By: YeonJuan <yeonjuan93@naver.com>

* Update docs/rules/no-empty-function.md

Co-Authored-By: YeonJuan <yeonjuan93@naver.com>

* Update docs/rules/no-empty-function.md

Co-Authored-By: YeonJuan <yeonjuan93@naver.com>

* Docs: fixed the ecma version for docs snippet

Co-authored-by: YeonJuan <yeonjuan93@naver.com>
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Sep 18, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Sep 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

no-empty-function false positive
4 participants