tools icon indicating copy to clipboard operation
tools copied to clipboard

feat(rome_js_analyze): new rule noCondAssign

Open 95th opened this issue 3 years ago • 1 comments

Summary

Implement eslint's no-cond-assign lint.

Test Plan

Added unit tests.

95th avatar Nov 15 '22 17:11 95th

Deploy Preview for docs-rometools ready!

Built without sensitive environment variables

Name Link
Latest commit cfea4dc52414f2829ad731ac352b1c42ffaa75bc
Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/6374c964eaa9040009cea249
Deploy Preview https://deploy-preview-3750--docs-rometools.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify[bot] avatar Nov 15 '22 17:11 netlify[bot]

Should this rule be named noConditionalAssignment to avoid abbreviation

Utilize verbosity when naming commands and flags. No unnecessary and confusing abbreviations. source

CC: @ematipico @leops

MichaReiser avatar Nov 16 '22 08:11 MichaReiser

!bench_analyzer

MichaReiser avatar Nov 16 '22 09:11 MichaReiser

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.00      2.1±0.02ms     5.5 MB/sec    1.00      2.1±0.02ms     5.5 MB/sec
analyzer/index.js         1.00      6.1±0.03ms     5.4 MB/sec    1.00      6.1±0.07ms     5.4 MB/sec
analyzer/lint.ts          1.00      3.0±0.03ms    14.1 MB/sec    1.00      3.0±0.03ms    14.0 MB/sec
analyzer/parser.ts        1.00      7.3±0.04ms     6.7 MB/sec    1.00      7.2±0.10ms     6.7 MB/sec
analyzer/router.ts        1.00      5.1±0.02ms    11.9 MB/sec    1.00      5.1±0.02ms    11.9 MB/sec
analyzer/statement.ts     1.00      7.1±0.01ms     5.0 MB/sec    1.00      7.1±0.05ms     5.0 MB/sec
analyzer/typescript.ts    1.00     10.7±0.03ms     5.1 MB/sec    1.00     10.7±0.07ms     5.1 MB/sec

github-actions[bot] avatar Nov 16 '22 09:11 github-actions[bot]

Given the nature of the rule, I think we could provide a suggested fix to replace the assignment expression with a === binary expression

It could be incorrect in some cases. For example, traversing a linked list, you could have

do {} while (node = node.next)

Suggesting the below seem bad:

do {} while (node === node.next)

95th avatar Nov 16 '22 10:11 95th

Given the nature of the rule, I think we could provide a suggested fix to replace the assignment expression with a === binary expression

It could be incorrect in some cases

I think that's acceptable as long it's only a "suggested fix" with Applicability::MaybeIncorrect, as the name indicates it's allowed to be incorrect and should not be applied automatically without a manual review by the user unlike a "safe fix" with Applicability::Always

leops avatar Nov 16 '22 10:11 leops

@leops Is it ok if I take it up in a followup PR?

95th avatar Nov 16 '22 11:11 95th

Is it ok if I take it up in a followup PR?

Sure yes let's get this merged first

leops avatar Nov 16 '22 13:11 leops