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

Jump to bottom

Fix: require-await ignore async generators (fixes #12459) #13048

Merged
merged 4 commits into from Mar 28, 2020
Merged

Fix: require-await ignore async generators (fixes #12459) #13048

merged 4 commits into from Mar 28, 2020

Conversation

anikethsaha
Copy link
Member

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)

[X] Documentation update
[ ] 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:

Fixes #12459

What changes did you make? (Give an overview)

after this change, require-await rule won't check for async generator functions . So an async generator function having no await wont report any error

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

#12459

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Mar 15, 2020
@anikethsaha anikethsaha changed the title Chore: ignoring async generator function by default Chore: ignoring async generator function by default (fixes #12459) Mar 17, 2020
@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 17, 2020
kaicataldo
kaicataldo suggested changes Mar 17, 2020
View reviewed changes
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.

Thanks for working on this!

docs/rules/require-await.md Outdated Show resolved Hide resolved
tests/lib/rules/require-await.js Outdated Show resolved Hide resolved
@kaicataldo
Copy link
Member

kaicataldo commented Mar 17, 2020 •
edited

Also, can we change the commit message from Chore: ignoring async generator function by default (fixes #12459) to Fix: require-await should ignore async generators (fixes #12459)?

@anikethsaha
Copy link
Member Author

Also, can we change the commit message from Chore: ignoring async generator function by default (fixes #12459) to Fix: require-await should ignore async generators (fixes #12459)?

ok I will do that 👍

@kaicataldo kaicataldo changed the title Chore: ignoring async generator function by default (fixes #12459) Fix: require-await should ignore async generators (fixes #12459) Mar 20, 2020
kaicataldo
kaicataldo approved these changes Mar 20, 2020
View reviewed changes
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.

One small typo in the docs, but otherwise LGTM!

docs/rules/require-await.md Outdated Show resolved Hide resolved
@anikethsaha @kaicataldo
Docs: fixed typo for generator to generators
2a90a3e 
Co-Authored-By: Kai Cataldo <kai@kaicataldo.com>
mdjermanovic
mdjermanovic requested changes Mar 21, 2020
View reviewed changes
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.

Can you please add one valid test without yield * (and without await of course).

From #12459 it isn't entirely clear should the rule ignore all async generators or only those with yield *.

I believe the decision was to ignore all async generators, so it would be nice to have a test case that shows this intention.

@anikethsaha anikethsaha changed the title Fix: require-await should ignore async generators (fixes #12459) Fix: require-await ignore async generators (fixes #12459) Mar 21, 2020
@anikethsaha
Copy link
Member Author

anikethsaha commented Mar 21, 2020 •
edited

cc @mdjermanovic I added few tests without yield *

Not sure why PR title is not passing !

@anikethsaha anikethsaha reopened this Mar 21, 2020
@mdjermanovic mdjermanovic changed the title Fix: require-await ignore async generators (fixes #12459) Fix: require-await ignore async generators (fixes #12459) Mar 21, 2020
@mdjermanovic mdjermanovic changed the title Fix: require-await ignore async generators (fixes #12459) Fix: require-await ignore async generators (fixes #12459) Mar 21, 2020
@mdjermanovic
Copy link
Member

Not sure why PR title is not passing !

PR title had extra spaces at start/end.

@anikethsaha
Copy link
Member Author

Not sure why PR title is not passing !

PR title had extra spaces at start/end.

oh, my bad !

thanks 👍

mdjermanovic
mdjermanovic approved these changes Mar 21, 2020
View reviewed changes
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!

@bradzacher
Copy link
Contributor

bradzacher commented Mar 23, 2020 •
edited

Is this a duplicate of #12484?

@anikethsaha
Copy link
Member Author

ohh, completely missed that. I think yeah...

I think it's better to ping in that first to know if he is still active and wants that to be landed. I will close this one then. Else we can go with this if that seems in-active

cc @mdjermanovic @kaicataldo

@kaicataldo kaicataldo mentioned this pull request Mar 28, 2020
Closed
@kaicataldo
Copy link
Member

@bradzacher Thanks for pointing that out. Given the lack of response from the author of the other PR, I have closed it in favor of this one.

@kaicataldo kaicataldo merged commit eb1a43c into eslint:master Mar 28, 2020
@kaicataldo
Copy link
Member

Thanks for contributing!

@anikethsaha anikethsaha deleted the fixes-12459 branch March 28, 2020 06:51
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Sep 25, 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 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kaicataldo kaicataldo kaicataldo approved these changes

@mdjermanovic mdjermanovic mdjermanovic approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

require-await should be disabled for async generators
4 participants
@anikethsaha @kaicataldo @mdjermanovic @bradzacher