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

feat(eslint-plugin): add rule `no-confusing-void-expression` #2605

Merged
merged 26 commits into from Nov 2, 2020

Conversation

@phaux
Copy link
Contributor

@phaux phaux commented Sep 27, 2020

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:

  • More descriptive messages for the common cases.
    These cases are: returning void from arrow function shorthand; and early return with void function from other void function.
  • Provides autofix for the common cases.
    These usually aren't mistakes, but only confusing code written by developers who themselves know what they're doing.
  • Ignores only the RHS of the short-circuiting operators, instead of both operands.
    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.
  • Rename ignore-arrow-function-shorthand option to ignoreArrowShorthand.
    It's less verbose but still obvious enough and it's camelCase.
  • Extra option: ignoreVoidOperator similar to no-misused-promises ignoreVoid option.
    Allow silencing the error by using the void operator and make it explicit in the code that you actually want this behavior.

Fixes #2425

@typescript-eslint
Copy link

@typescript-eslint typescript-eslint bot commented Sep 27, 2020

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.

Copy link
Contributor

@tadhgmister tadhgmister left a comment

The fact that you violate this rule with it's own implementation is very silly, do you intent to leave it like that?

@phaux phaux force-pushed the phaux:no-void-expr branch from e98f0cc to d6df362 Sep 28, 2020
@codecov
Copy link

@codecov codecov bot commented Sep 29, 2020

Codecov Report

Merging #2605 into master will increase coverage by 0.04%.
The diff coverage is 97.89%.

@@            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     
Flag Coverage Δ
unittest 92.79% <97.89%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/eslint-plugin/src/configs/all.ts 100.00% <ø> (ø)
...lugin/src/rules/consistent-indexed-object-style.ts 88.88% <80.00%> (-1.36%) ⬇️
...t-plugin/src/rules/no-confusing-void-expression.ts 98.82% <98.82%> (ø)
packages/eslint-plugin/src/rules/no-shadow.ts 94.04% <100.00%> (+0.37%) ⬆️
Copy link
Member

@bradzacher bradzacher left a 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)
@phaux
Copy link
Contributor Author

@phaux phaux commented Oct 4, 2020

Updated ROADMAP and left the rule name as is for now. I don't have a strong preference about the name.

@bradzacher bradzacher changed the title feat(eslint-plugin): new rule: no-void-expression feat(eslint-plugin): add rule `no-void-expression` Oct 25, 2020
@bradzacher
Copy link
Member

@bradzacher bradzacher commented Oct 25, 2020

The code itself LGTM - I think the name should be changed to be clearer.

I'm okay with any of these two names:

  • no-void-assignment
  • no-void-type-assignment
  • no-void-typed-assignment

Change the name and fix the merge conflict and we can merge this!
Thanks for your work

@phaux phaux force-pushed the phaux:no-void-expr branch from f53971f to 9596ef5 Nov 1, 2020
phaux added 3 commits Nov 1, 2020
@phaux
Copy link
Contributor Author

@phaux phaux commented Nov 1, 2020

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:

  • no-undefined-expression - makes it clear that it's not about the void operator.
  • restrict-void-expressions - doesn't tell much, but at least it's not misleading. (I vote for this one)
  • require-void-expression-as-statement - very verbose but it's literally part of the rule's description and it's obvious.
@bradzacher
Copy link
Member

@bradzacher bradzacher commented Nov 1, 2020

no-undefined-expression

This one isn't quite right, because undefined and void are different.


expression is somewhat broad because this rule is only checking the results function-like calls.
I.e.

declare const x: {
    a: void
};
const y = x.a; // this will be ignored by the rule

It 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 no-void-expressions 😄 )
Choose whatever you like, and we're good to go!

@phaux phaux force-pushed the phaux:no-void-expr branch from 37843b1 to 29fca40 Nov 2, 2020
Copy link
Member

@bradzacher bradzacher left a comment

LGTM - Thanks for your work as always!

@bradzacher bradzacher merged commit c8a4dad into typescript-eslint:master Nov 2, 2020
8 checks passed
8 checks passed
Typecheck
Details
Unit tests Unit tests
Details
Code style and lint
Details
Run integration tests on primary Node.js version
Details
Run unit tests on other Node.js versions (10.x)
Details
Run unit tests on other Node.js versions (14.x)
Details
Publish the latest code as a canary version
Details
Semantic Pull Request ready to be squashed
Details
@glen-84
Copy link
Contributor

@glen-84 glen-84 commented Nov 10, 2020

@JamesHenry,

The title of this PR was not updated to reflect the new rule name (no-confusing-void-expression), so the release notes refer to this incorrect name.

@bradzacher bradzacher changed the title feat(eslint-plugin): add rule `no-void-expression` feat(eslint-plugin): add rule `no-confusing-void-expression` Nov 10, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.