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

fix(lint/useExponentiationOperator): avoid incorrect code fixes #121

Merged
merged 1 commit into from
Sep 4, 2023

Conversation

Conaclos
Copy link
Member

@Conaclos Conaclos commented Sep 2, 2023

Summary

Fix rome#3850.

  • Handle pre-update

    Previously, Biome suggested an incorrect code fix for the following example:

    - 1+Math.pow(++a, 2)
    + 1+++a**2

    rome#3850 proposed adding a space between + and ++ to mimic the behavior of ESLint.

    However, Biome and Prettier automatically add parentheses around ++a.
    Thus, I decided to provide the following suggestion:

    - 1+Math.pow(++a, 2)
    + 1+(++a) ** 2
  • The code fix now preserves any parentheses around the arguments.

    - Math.pow((1), (2))
    + (1) ** (2)
  • The code fix now adds spaces around **.

  • Previously the rule didn't suggest any code fix if a leading or trailing comment was present.
    The rule now provides a suggestion (and keeps the leading and trailing comments)

    - /* leading comment */ Math.pow(2, 2) // trailing comment
    + /* leading comment */ 2 ** 2 // trailing comment
  • Previously the rule didn't suggest any code fix if inner comments was removed.
    Now the rule makes suggestions even if this removes inner comments.
    I think it is ok because the rule is marked as unsafe.

    - Math.pow(/*a*/ 2, /*b*/ 2 /*c*/)
    + 2 ** 2

    An alternative is to transfer all inner comments:

    /*a*/ 2 ** /*b*/ 2 /*c*/

    Any thoughts?
    EDIT: I finally decided to choose the alternative, i.e. keeping the comments.

I also take the opportunity to remove all unnecessary cloning.

Test Plan

Updated tests.

@Conaclos Conaclos temporarily deployed to Website deployment September 2, 2023 12:10 — with GitHub Actions Inactive
@github-actions github-actions bot added A-Linter Area: linter A-Parser Area: parser L-JavaScript Language: JavaScript and super languages labels Sep 2, 2023
@Conaclos Conaclos force-pushed the conaclos/lint/useExponentiationOperator/rome3850 branch from 27537a2 to aee2acb Compare September 2, 2023 14:42
@Conaclos Conaclos temporarily deployed to Website deployment September 2, 2023 14:42 — with GitHub Actions Inactive
@Conaclos Conaclos force-pushed the conaclos/lint/useExponentiationOperator/rome3850 branch from aee2acb to 19479a4 Compare September 2, 2023 14:50
@Conaclos Conaclos temporarily deployed to Website deployment September 2, 2023 14:50 — with GitHub Actions Inactive
@github-actions github-actions bot added the A-Website Area: website label Sep 2, 2023
@Conaclos Conaclos marked this pull request as ready for review September 2, 2023 14:51
@Conaclos Conaclos requested a review from ematipico September 2, 2023 14:51
@Conaclos Conaclos force-pushed the conaclos/lint/useExponentiationOperator/rome3850 branch from 19479a4 to dc3f4dc Compare September 2, 2023 15:41
@Conaclos Conaclos temporarily deployed to Website deployment September 2, 2023 15:41 — with GitHub Actions Inactive
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Left some suggestion

@Conaclos Conaclos force-pushed the conaclos/lint/useExponentiationOperator/rome3850 branch from dc3f4dc to c8e51be Compare September 4, 2023 13:59
@github-actions github-actions bot added the A-Tooling Area: internal tools label Sep 4, 2023
@Conaclos Conaclos force-pushed the conaclos/lint/useExponentiationOperator/rome3850 branch 3 times, most recently from 4ee9068 to 9ae1b88 Compare September 4, 2023 15:07
@Conaclos Conaclos temporarily deployed to Website deployment September 4, 2023 15:07 — with GitHub Actions Inactive
@github-actions github-actions bot added A-Changelog Area: changelog and removed A-Tooling Area: internal tools labels Sep 4, 2023
@Conaclos Conaclos changed the title refactor(lint/useExponentiationOperator): avoid incorrect code fixes fix(lint/useExponentiationOperator): avoid incorrect code fixes Sep 4, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 4, 2023

Parser conformance results on ubuntu-latest

js/262

Test result main count This PR count Difference
Total 48863 48863 0
Passed 47808 47808 0
Failed 1055 1055 0
Panics 0 0 0
Coverage 97.84% 97.84% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 40 40 0
Passed 37 37 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.50% 92.50% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 6212 6212 0
Passed 1764 1764 0
Failed 4448 4448 0
Panics 0 0 0
Coverage 28.40% 28.40% 0.00%

ts/babel

Test result main count This PR count Difference
Total 639 639 0
Passed 573 573 0
Failed 66 66 0
Panics 0 0 0
Coverage 89.67% 89.67% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 17224 17224 0
Passed 13121 13121 0
Failed 4103 4103 0
Panics 0 0 0
Coverage 76.18% 76.18% 0.00%

@Conaclos Conaclos merged commit 9649616 into main Sep 4, 2023
@Conaclos Conaclos deleted the conaclos/lint/useExponentiationOperator/rome3850 branch September 4, 2023 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-Linter Area: linter A-Parser Area: parser A-Website Area: website L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

📎 useExponentiationOperator
2 participants