feat(eslint-plugin): [no-unused-expressions] extend for optional chaining #1175
Conversation
|
Thanks for the PR, @Nizarius! 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. |
There was a problem hiding this comment.
it looks like you've copied most of the code from the existing rule implementation.
May I suggest that you instead build on top of the existing implementation?
Duplicating the implementation means we have to keep it in sync with eslint, which is a huge maintenance burden.
You could do something similar to
https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/src/rules/camelcase.ts
I.e. - defer most of the processing to the base rule's handler, but add handling which will handle optional chaining:
const rules = baseRule.create(context);
ExpressionStatement(node) {
if (isValidOptionalChain(node)) {
return;
}
rules.ExpressionStatement(node);
}|
@bradzacher yeah, great idea! I will do it, but it looks like I would still need a lot of duplicating code as I can't get private functions from eslint rule. |
|
@bradzacher done. Can you tell me how I should add docs to that rule (just copy it from eslint or other way?) |
There was a problem hiding this comment.
Don't forget to add a doc for the new rule.
Have a look at the docs for semi
https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/semi.md
Essentially with rule extensions now, we like our extension rule docs to just mention the changes it makes to the rule, and then link out to the original eslint docs
packages/eslint-plugin/tests/rules/no-unused-expressions.test.ts
Outdated
Show resolved
Hide resolved
dc03880 to
daa4668
Compare
|
@bradzacher added docs and new tests. Could you explain to me how I should update documentation parsing to avoid 'Incorrect line number for' errors? Also, should I add this rule to recommended ones to prevent errors with base configuration? |
The rule needs to be listed in the plugin readme. The table should be sorted alphabetically.
Changes to the recommended config are considered breaking changes, so we cannot do that for now. |
Codecov Report
@@ Coverage Diff @@
## master #1175 +/- ##
==========================================
+ Coverage 93.96% 93.98% +0.01%
==========================================
Files 120 121 +1
Lines 5207 5217 +10
Branches 1443 1444 +1
==========================================
+ Hits 4893 4903 +10
Misses 179 179
Partials 135 135
|
|
@JamesHenry This isn't in the release notes? |
|
@glen-84 it was released earlier - in 2.7 |
|
🤦♂ I must have missed that. |
The default ESLint `no-unused-expressions` rule cannot handle optional chaining in TypeScript, for example: ``` onChangeHook?.(nextValue); ``` In typescript-eslint/typescript-eslint#1175 the base rule was duplicated in to a TypeScript-specific one that handles optional chaining correctly. Presumably this will eventually be merged back to the base rule as this is now part of the ECMAScript standard.
The default ESLint `no-unused-expressions` rule cannot handle optional chaining in TypeScript, for example: ``` onChangeHook?.(nextValue); ``` In typescript-eslint/typescript-eslint#1175 the base rule was duplicated in to a TypeScript-specific one that handles optional chaining correctly. Presumably this will eventually be merged back to the base rule as this is now part of the ECMAScript standard.
Fixes #1138
To note: this PR depends on #1169, so it's better to watch after first one