-
-
Notifications
You must be signed in to change notification settings - Fork 65
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
Remove rule @typescript-eslint/no-empty-function
#248
Comments
Seems reasonable to me. I think the first step would be to check upstream if we can include the rule there though 🤔 Regarding |
I was thinking something along "surely not many projects at all have empty functions", but I was very wrong 😅
I think it's reasonable to disable this, as I don't see that upstream will enable it, and I think that we should follow upstream 👍 |
Thank you, @swansontec. Since your main example is non-render react components and since @LinusU reports that in TypeScript those should return In general, we follow standard, but it seems like empty functions are less tolerated by TypeScript itself. And the edge case could be a good candidate for an eslint-disable comment. |
I don't think we have any React projects in the Standard tests, most that I looked at wasn't React specific at least.
I'm not sure what you mean about this? It's perfectly valid to have an empty function, and TypeScript will correctly infer that function to return You can also pass empty functions to any receiver expecting a function that returns I think that we should try and follow upstream as closely as possible |
Oops, sorry for the bad example! I've been focusing on our backend recently, and I guess my memory got a little blurry. I realize this is moot, since the issue is closed, but here are two genuine examples for the record: export const silentLogger: EdgeLogger = {
info () {},
warn () {},
error () {}
} function doNothing (): void {}
// Later:
const {
onAddressesChecked = doNothing,
onBalanceChanged = doNothing,
} = this.props
// Now we can use these callbacks without worry, since they always exist... |
For the latter, I personally use But the first example is a very good one, and I think that there was a lot of that in the packages that wasn't compatible in the ecosystem |
I agree, that's probably a good cleanup, and best for all new code! However, I wrote this particular code in 2017, but optional chaining only came out in the latest version of Typescript. |
Lint-disabling comments are a legitimate part of code. Demonstrating a legitimate false-positive case for a rule does not negate the value of the rule. It just shows an exception, in this case, that should be handled with a disable comment, if you ask me. |
That ~25% of the most popular packages that uses standard are using empty functions leads me to believe that this is more than an anomaly. But it would be interesting to look at the cases in depth and see how they are using it, and if there is a better way. That said, I think that this discussion should be continued in the normal Standard repo since the rules should be enabled there first |
What version of this package are you using?
v14
What problem do you want to solve?
The upstream Standard.js project has access to the
no-empty-function
rule in plain-old ESLint, but has chosen not to activate it.If this rule seems useful, then whoever wants to champion it should start a discussion on upstream Standard.js. If upstream adopts it, then this repo should continue to include it. If they don't adopt it, then this repo should remove it.
I realize that eslint-config-standard-with-typescript includes many rules that upstream does not, since we have access to type information. However, in cases where there is a 1:1 match between the Typescript rule and the ESLint rule, I think we should leave the decision to upstream, since it's just as valuable or harmful there as it is here.
What do you think is the correct solution to this problem?
Remove
@typescript-eslint/no-empty-function
.After living with it for a while, I personally find this rule to be quite unhelpful. I write a lot of React code, where it's common to have do-nothing class methods like
render()
for components that don't draw to the screen. It's perfectly obvious to what is going on in these cases, and yet this rule requires me to put in an pointless comment. That is why I don't really want to open the discussion at the Standard.js level, but will leave that for others to do.Are you willing to submit a pull request to implement this change?
Of course.
The text was updated successfully, but these errors were encountered: