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

feat(rome_js_analyze): noExtraBooleanCast #2868

Merged
merged 17 commits into from
Aug 3, 2022
Merged

feat(rome_js_analyze): noExtraBooleanCast #2868

merged 17 commits into from
Aug 3, 2022

Conversation

IWANABETHATGUY
Copy link
Contributor

Summary

  1. Resolved noExtraBooleanCast #2660

Test Plan

  1. cargo test

@IWANABETHATGUY IWANABETHATGUY marked this pull request as ready for review July 13, 2022 05:55
@IWANABETHATGUY IWANABETHATGUY requested a review from a team July 13, 2022 05:55
@IWANABETHATGUY IWANABETHATGUY force-pushed the feat/no-extra-boolean-cast branch from 47347ec to d416666 Compare July 13, 2022 05:57
xunilrj
xunilrj reviewed Jul 13, 2022
View reviewed changes
xunilrj
xunilrj reviewed Jul 13, 2022
View reviewed changes
xunilrj
xunilrj reviewed Jul 13, 2022
View reviewed changes
xunilrj
xunilrj reviewed Jul 13, 2022
View reviewed changes
xunilrj
xunilrj reviewed Jul 13, 2022
View reviewed changes
xunilrj
xunilrj reviewed Jul 13, 2022
View reviewed changes
xunilrj
xunilrj reviewed Jul 13, 2022
View reviewed changes
ematipico
ematipico reviewed Jul 13, 2022
View reviewed changes
ematipico
ematipico reviewed Jul 14, 2022
View reviewed changes
ematipico
ematipico suggested changes Jul 14, 2022
View reviewed changes
crates/rome_js_analyze/src/analyzers/js/no_extra_boolean_cast.rs
pub(crate) NoExtraBooleanCast = "noExtraBooleanCast"
}

/// Check if this node is in the position of `test` slot of parent syntax node.
Copy link
Contributor

@ematipico ematipico Jul 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Could you please review this comment? What's test? The reason I am asking to change it is because if a new developer reads this documentation, they won't understand what's test because it's really specific to the implementation of the function. It's better to give an overview of what the function does, not technical details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
Would it be better if I rename it to test clause of the parent

Copy link
Contributor

Choose a reason for hiding this comment

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

I think JsWhileStatement::test would be quite clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not about the name of the node, it's about the whole comment. When I read it without checking the code of function, I don't understand what it means.

@IWANABETHATGUY
Copy link
Contributor Author

!bench_analyzer

@github-actions
Copy link

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.00      2.9±0.03ms     4.0 MB/sec    1.03      3.0±0.01ms     3.9 MB/sec
analyzer/index.js         1.00      6.9±0.02ms     4.7 MB/sec    1.03      7.1±0.01ms     4.5 MB/sec
analyzer/lint.ts          1.00      4.8±0.01ms     8.7 MB/sec    1.03      4.9±0.01ms     8.5 MB/sec
analyzer/parser.ts        1.00     10.6±0.09ms     4.5 MB/sec    1.03     11.0±0.01ms     4.4 MB/sec
analyzer/router.ts        1.00      6.9±0.01ms     8.7 MB/sec    1.03      7.1±0.01ms     8.4 MB/sec
analyzer/statement.ts     1.00      9.1±0.01ms     3.9 MB/sec    1.03      9.4±0.01ms     3.8 MB/sec
analyzer/typescript.ts    1.00     13.3±0.03ms     4.1 MB/sec    1.03     13.7±0.04ms     4.0 MB/sec

@ematipico
Copy link
Contributor

@IWANABETHATGUY are there any progress on this PR? Do you need any help?

@IWANABETHATGUY
Copy link
Contributor Author

@IWANABETHATGUY are there any progress on this PR? Do you need any help?

Nope, I will address issues this week.

@IWANABETHATGUY
Copy link
Contributor Author

!bench_analyzer

@github-actions
Copy link

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.00      2.9±0.01ms     3.9 MB/sec    1.05      3.1±0.02ms     3.8 MB/sec
analyzer/index.js         1.00      6.0±0.05ms     5.3 MB/sec    1.05      6.3±0.04ms     5.1 MB/sec
analyzer/lint.ts          1.00      3.6±0.01ms    11.4 MB/sec    1.06      3.8±0.04ms    10.8 MB/sec
analyzer/parser.ts        1.00      9.2±0.04ms     5.2 MB/sec    1.05      9.7±0.09ms     5.0 MB/sec
analyzer/router.ts        1.00      6.3±0.03ms     9.8 MB/sec    1.05      6.6±0.03ms     9.3 MB/sec
analyzer/statement.ts     1.00      7.7±0.06ms     4.6 MB/sec    1.06      8.1±0.04ms     4.4 MB/sec
analyzer/typescript.ts    1.00     11.2±0.03ms     4.9 MB/sec    1.06     11.8±0.02ms     4.6 MB/sec

@IWANABETHATGUY
Copy link
Contributor Author

I think there should be a more performant way to implement this, let me try.

@ematipico
Copy link
Contributor

@xunilrj @leops could you please review and merge once you see fit?

@ematipico ematipico requested review from ematipico and xunilrj July 26, 2022 11:48
leops
leops approved these changes Jul 26, 2022
View reviewed changes
@IWANABETHATGUY IWANABETHATGUY force-pushed the feat/no-extra-boolean-cast branch from 3928494 to 00abf6b Compare July 26, 2022 13:44
@IWANABETHATGUY IWANABETHATGUY mentioned this pull request Aug 3, 2022
Closed
@ematipico ematipico added the A-Linter Area: linter label Aug 3, 2022
ematipico
ematipico reviewed Aug 3, 2022
View reviewed changes
crates/rome_js_analyze/src/analyzers/js/no_extra_boolean_cast.rs Outdated
/// !!x;
/// ```
pub(crate) NoExtraBooleanCast {
version: "0.8.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be 0.9.0 now. Once updated, we can merge it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@IWANABETHATGUY IWANABETHATGUY force-pushed the feat/no-extra-boolean-cast branch from 00abf6b to d6cc2b9 Compare August 3, 2022 08:31
@ematipico ematipico merged commit d271928 into rome:main Aug 3, 2022
@IWANABETHATGUY IWANABETHATGUY deleted the feat/no-extra-boolean-cast branch August 3, 2022 09:26
IWANABETHATGUY added a commit to IWANABETHATGUY/tools that referenced this pull request Aug 22, 2022
@IWANABETHATGUY
feat(rome_js_analyze): noExtraBooleanCast (rome#2868)
61a72e7
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ematipico ematipico ematipico approved these changes

@leops leops leops approved these changes

@MichaReiser MichaReiser Awaiting requested review from MichaReiser

@xunilrj xunilrj Awaiting requested review from xunilrj

Assignees
No one assigned
Labels
A-Linter Area: linter
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

noExtraBooleanCast
4 participants
@IWANABETHATGUY @ematipico @xunilrj @leops
点击 这是indexloc提供的php浏览器服务，不要输入任何密码和下载