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

ESLint Config Migration: Turn off consistent-return from linting rules #1049

Closed
wants to merge 1 commit into from

Conversation

filmaj
Copy link
Contributor

@filmaj filmaj commented Aug 9, 2021

Summary

This is a PR that should be merged into #1024 and incrementally addresses #842.

This PR turns off the consistent-return rule.

In mixed async + Promise code, it can be a pain to adhere to the rule. For an example, see the stop method of HTTPReceiver.ts. Both the outer Promise as well as the inner server.close methods raise the following lint failure:

  254:42  error    Expected to return a value at the end of arrow function          consistent-return
  258:33  error    Expected to return a value at the end of arrow function          consistent-return

What does everyone think? Is there a better way to implement the stop method without violating this rule?

Impact

Before

✖ 362 problems (173 errors, 189 warnings)
  76 errors and 0 warnings potentially fixable with the `--fix` option.

After

✖ 353 problems (164 errors, 189 warnings)
  76 errors and 0 warnings potentially fixable with the `--fix` option.

Requirements (place an x in each [ ])

@filmaj filmaj added the tests M-T: Testing work only label Aug 9, 2021
@filmaj filmaj self-assigned this Aug 9, 2021
@filmaj filmaj changed the title Turn off consistent-return from linting rules ESLint Config Migration: Turn off consistent-return from linting rules Aug 9, 2021
…ise code, it can be a pain to adhere to the rule
@seratch
Copy link
Member

seratch commented Aug 10, 2021

For other use cases described in the rule document, I still see some value in consistent-return. Thus, I would like to explore alternative ways to avoid disabling this rule to all the code in the repo. However, I'm also not sure about the ways to settle this lint error in the stop() method. How about disabling this rule only for the stop() methods and other similar as long as they are not hundreds?

@filmaj
Copy link
Contributor Author

filmaj commented Aug 10, 2021

@seratch that sounds good. There are 9 occurrences of this rule failing, and from what I could tell, they are all in the various Receiver implementations, all follow the same general pattern seen by the stop example.

I think one possible solution would be to ensure that both the Receiver.stop and the underlying node HTTP server.close methods return a Promise, then we can pass Promises around to, I think, pass this rule. I can try my hand at an implementation tomorrow.

@seratch
Copy link
Member

seratch commented Aug 10, 2021

Sounds great! Thanks for your careful work here.

@filmaj
Copy link
Contributor Author

filmaj commented Aug 23, 2021

@seratch I attempted to implement fixes for this rule instead of removal in #1075. Perhaps that is a better choice than this PR.

@seratch
Copy link
Member

seratch commented Aug 24, 2021

@filmaj Yes, I agree. Let go with #1075 instead

@filmaj
Copy link
Contributor Author

filmaj commented Aug 24, 2021

Closing in favour of #1075

@filmaj filmaj closed this Aug 24, 2021
@filmaj filmaj deleted the no-consistent-return branch August 24, 2021 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests M-T: Testing work only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants