Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feat(rome_js_analyze): new rule noCondAssign #3750

Merged
merged 7 commits into from
Nov 16, 2022
Merged

feat(rome_js_analyze): new rule noCondAssign #3750

merged 7 commits into from
Nov 16, 2022

Conversation

95th
Copy link
Contributor

@95th 95th commented Nov 15, 2022

Summary

Implement eslint's no-cond-assign lint.

Test Plan

Added unit tests.

@95th 95th requested a review from a team November 15, 2022 17:17
@netlify
Copy link

netlify bot commented Nov 15, 2022

Deploy Preview for docs-rometools ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit cfea4dc
🔍 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.

Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see how straightforward the implementation is thanks to the separation of expression/assignments in the AST.

@MichaReiser MichaReiser added A-Linter Area: linter L-JavaScript Langauge: JavaScript labels Nov 16, 2022
@MichaReiser MichaReiser added this to the 11.0.0 milestone Nov 16, 2022
@MichaReiser
Copy link
Contributor

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
Copy link
Contributor

!bench_analyzer

@github-actions
Copy link

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

Copy link
Contributor

@leops leops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@95th
Copy link
Contributor Author

95th commented Nov 16, 2022

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)

@leops
Copy link
Contributor

leops commented Nov 16, 2022

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

@95th
Copy link
Contributor Author

95th commented Nov 16, 2022

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

@leops
Copy link
Contributor

leops commented Nov 16, 2022

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

Sure yes let's get this merged first

@MichaReiser MichaReiser merged commit 3129dbd into rome:main Nov 16, 2022
@95th 95th deleted the no_cond_assign branch November 17, 2022 00:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Linter Area: linter L-JavaScript Langauge: JavaScript
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants