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

Rule Change: make no-return-await suggestable #16632

Closed
1 task done
dbartholomae opened this issue Dec 8, 2022 · 12 comments · Fixed by #16637
Closed
1 task done

Rule Change: make no-return-await suggestable #16632

dbartholomae opened this issue Dec 8, 2022 · 12 comments · Fixed by #16637
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 enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects

Comments

@dbartholomae
Copy link
Contributor

What rule do you want to change?

no-return-await

What change to do you want to make?

Implement autofix

How do you think the change should be implemented?

A new option

Example code

async function foo {
  return await Promise.resolve("bar")
}

What does the rule currently do for this code?

// Error, but don't change:
async function foo {
return await Promise.resolve("bar")
}

What will the rule do after it's changed?

// Fix it
async function foo {
return Promise.resolve("bar")
}

Participation

  • I am willing to submit a pull request to implement this change.

Additional comments

Hi! There have been discussions about whether no-return-await should be a core rule. These are resolved - and, consequently, I would argue that the rule should be auto-fixable.

@dbartholomae dbartholomae added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules labels Dec 8, 2022
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Dec 8, 2022
@yeonjuan
Copy link
Member

yeonjuan commented Dec 8, 2022

Hi @dbartholomae This proposal has been made in the past (#7593).
And it was not accepted due to the removing await can change the behavior of code.

@mdjermanovic mdjermanovic moved this from Needs Triage to Triaging in Triage Dec 8, 2022
@mdjermanovic mdjermanovic moved this from Triaging to Feedback Needed in Triage Dec 8, 2022
@mdjermanovic
Copy link
Member

I agree with @yeonjuan, per #7593 (comment) this doesn't seem safe for autofixing.

Perhaps we could consider implementing this as suggestions?

@dbartholomae
Copy link
Contributor Author

Thanks, I didn't find that issue when searching.

@dbartholomae
Copy link
Contributor Author

Happy to close this, unless it makes sense to discuss implementing this as a suggestion here

@nzakas
Copy link
Member

nzakas commented Dec 9, 2022

I'd be fine with implementing a suggestion. Just so everyone is aware: suggestions have no effect on the command line, they are just displayed inside of editors like Visual Studio Code as potential fixes. If that's what you're interested in, then we can move forward.

@dbartholomae
Copy link
Contributor Author

Is it possible to auto-fix suggestions via eslint --fix --fix-type suggestion? I'm looking for a way to autofix this in a large codebase, so suggestions without a way to auto fix wouldn't help me and then I would implement a codemod instead.

@dbartholomae
Copy link
Contributor Author

If there is interest in some way to auto-fix suggestions, that would also be something that I would be willing to contribute - or set up as a separate open-source repo that uses eslint machine-readable output.

@nzakas
Copy link
Member

nzakas commented Dec 9, 2022

No, there is no way to autofix suggestions. The --fix-type option is unfortunately named because it means fixing rules that are described as suggestions rather than editor suggestions. A naming collision we can't quite escape.

And no, we don't want to auto-fix suggestions in the core. As already explained, autofixes must not change the meaning of the code being fixed as otherwise there can be side effects that are difficult to identify.

@dbartholomae
Copy link
Contributor Author

Thanks! I haven't tried it yet, but it seems that eslint-interactive already allows to auto-fix suggestions. I'll check it, and if it works, I would love to contribute a suggestion for this rule :)

@dbartholomae
Copy link
Contributor Author

Looks good! If you are interested, I'm happy to provide a PR on this. Thanks for all the support and the patience with my questions!

@dbartholomae dbartholomae changed the title Rule Change: make no-return-await fixable Rule Change: make no-return-await suggestable Dec 9, 2022
@mdjermanovic mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label Dec 9, 2022
@mdjermanovic mdjermanovic moved this from Feedback Needed to Ready to Implement in Triage Dec 9, 2022
@mdjermanovic
Copy link
Member

Yes, PR is welcome!

@dbartholomae
Copy link
Contributor Author

I've opened #16637 to fix this.

@harish-sethuraman harish-sethuraman linked a pull request Dec 10, 2022 that will close this issue
1 task
@mdjermanovic mdjermanovic moved this from Ready to Implement to Pull Request Opened in Triage Dec 10, 2022
Triage automation moved this from Pull Request Opened to Complete Dec 16, 2022
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jun 15, 2023
@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 Jun 15, 2023
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 enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
Archived in project
Triage
Complete
Development

Successfully merging a pull request may close this issue.

4 participants