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

Potentially incorrect documentation example for no-return-await #13656

Closed
cyberwombat opened this issue Sep 5, 2020 · 4 comments · Fixed by #13657
Closed

Potentially incorrect documentation example for no-return-await #13656

cyberwombat opened this issue Sep 5, 2020 · 4 comments · Fixed by #13657
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 documentation Relates to ESLint's documentation rule Relates to ESLint's core rules

Comments

@cyberwombat
Copy link

cyberwombat commented Sep 5, 2020

According to the docs: https://github.com/eslint/eslint/blob/master/docs/rules/no-return-await.md the following are not essentially equivalent.

Example of invalid usage:

async function foo() {
    return await bar();
}

Example of valid usage:

async function foo() {
    const x = await bar();
    return x;
}

That makes no sense. Neither is proper usage. Is this a documentation bug or am I misunderstanding why it's there (stack trace is all I can think of)

@cyberwombat cyberwombat added bug ESLint is working incorrectly triage An ESLint team member will look at this issue soon labels Sep 5, 2020
@mdjermanovic mdjermanovic added documentation Relates to ESLint's documentation evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules and removed bug ESLint is working incorrectly triage An ESLint team member will look at this issue soon labels Sep 5, 2020
@mdjermanovic
Copy link
Member

Hi @cyberwombat, thanks for the issue!

Examples of correct code do not aim to imply that it is a "valid usage". It's just a lint-free code for the rule.

The const example is correctly in "Examples of correct code for this rule" because this rule checks only return statements, so it doesn't report that code.

Maybe we should add a comment above this example, to avoid confusion?

@cyberwombat
Copy link
Author

@mdjermanovic I see. Yes I think a comment would be in order. I posted this as the result of a lengthy reddit discussion where we are wondering why eslint is saying they are not essentially the same. I'd recommend removing it from the "valid" section and adding it below with a mention that while it's incorrect eslint will not check it. What do you think?

@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Sep 5, 2020
@mdjermanovic
Copy link
Member

I made a docs PR #13657.

It seemed a bit complicated to separate this example. We can't formulate that it's "incorrect" in general because the document also lists some use cases in favor of return await (in the opening section and in the When Not To Use It section). It also isn't incorrect by this rule (it does look incorrect if we follow the logic of the rule, but the rule still doesn't report it), so it seemed best to just leave the example where it was, with an added comment.

@cyberwombat
Copy link
Author

Great. sounds good! I'll let you close this - dunno if it needs to stay open for the PR.

mdjermanovic added a commit that referenced this issue Sep 9, 2020
* Docs: clarify correct example in no-return-await (fixes #13656)

* Revert order of examples
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Mar 18, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Mar 18, 2021
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 documentation Relates to ESLint's documentation rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants