feat(eslint-plugin): add rule no-confusing-void-expression
#2605
Conversation
|
Thanks for the PR, @phaux! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day. |
Codecov Report
@@ Coverage Diff @@
## master #2605 +/- ##
==========================================
+ Coverage 92.75% 92.79% +0.04%
==========================================
Files 296 297 +1
Lines 9739 9833 +94
Branches 2733 2762 +29
==========================================
+ Hits 9033 9125 +92
Misses 332 332
- Partials 374 376 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Don't forget to update the ROADMAP appropriately
Do we want to rename the rule?
IMO no-void-expression is super confusing. To the uninitiated it sounds like it might be banning the void operator.
Possible alternatives:
no-void-assignment(my preference)no-void-return(probably too specific)no-void-usage(a little bit vague and confusing)
|
Updated ROADMAP and left the rule name as is for now. I don't have a strong preference about the name. |
no-void-expression
|
The code itself LGTM - I think the name should be changed to be clearer. I'm okay with any of these two names:
Change the name and fix the merge conflict and we can merge this! |
Co-authored-by: Tadhg McDonald-Jensen <tadhgmister@gmail.com>
Co-authored-by: Brad Zacher <brad.zacher@gmail.com>
Co-authored-by: Brad Zacher <brad.zacher@gmail.com>
Co-authored-by: Brad Zacher <brad.zacher@gmail.com>
Co-authored-by: Brad Zacher <brad.zacher@gmail.com>
Co-authored-by: Brad Zacher <brad.zacher@gmail.com>
This reverts commit 7d86695.
|
Before I change the name I'd like to propose these names as well. From my understanding the problem with the current name is that it can be interpreted as "ban the void operator" or "ban everything that evaluates to void". So how about:
|
This one isn't quite right, because
declare const x: {
a: void
};
const y = x.a; // this will be ignored by the ruleIt does leave us room to expand in future though. This is ultimately bike-shedding. I don't have that much of an opinion! I'm okay with any name you choose (except for the original |
|
The title of this PR was not updated to reflect the new rule name ( |
no-void-expressionno-confusing-void-expression
I used this rule with TSLint a lot so it makes sense that I'm responsible for porting it. I ported it from TSLint and then added some features:
These cases are: returning void from arrow function shorthand; and early return with void function from other void function.
These usually aren't mistakes, but only confusing code written by developers who themselves know what they're doing.
It only makes sense to put the side effect code into the right-hand side of
&&,||,??or ternary operator.That's why I decided to only allow void expressions in the RHS.
ignore-arrow-function-shorthandoption toignoreArrowShorthand.It's less verbose but still obvious enough and it's camelCase.
ignoreVoidOperatorsimilar tono-misused-promisesignoreVoidoption.Allow silencing the error by using the
voidoperator and make it explicit in the code that you actually want this behavior.Fixes #2425