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/useTemplate): preserve leading non-string addition #70

Merged
merged 1 commit into from
Aug 30, 2023

Conversation

Conaclos
Copy link
Member

@Conaclos Conaclos commented Aug 25, 2023

Summary

Fix rome#4713.
Previously, the rule made the following suggestion:

- a + b + "px"
+ `${a}${b}px`

This breaks code where a and b are numbers.
This PR fixes the issue making the following suggestion:

- a + b + "px"
+ `${a + b}px`

I take the opportunity to fix other unnoticed bugs:

  • we emitted DOLLAR_CURLY token instead of R_CURLY for the ending mark of a template element.
  • we erroneously handled tagged templates. They are now handled as non-string value

The rule also now provides the following suggestion (this aligns our behavior to the eslint behavior):

- v + "\n"
+ `${v}\n`

I also improved the code by avoiding recursions and reduced the number of traversals from 5 to 2.
The code to check if the binary operation is convertible to a template is now complete. This avoids to use memory in the case where the operation is not convertible (the majority of the situations).

Test Plan

New tests added.

@Conaclos Conaclos temporarily deployed to Website deployment August 25, 2023 15:57 — with GitHub Actions Inactive
@github-actions github-actions bot added A-CLI Area: CLI A-Linter Area: linter A-Parser Area: parser L-JavaScript Language: JavaScript and super languages labels Aug 25, 2023
@Conaclos Conaclos force-pushed the conaclos/lint/useTemplate/rome4713 branch from 2c2560b to 55b228c Compare August 26, 2023 22:23
@Conaclos Conaclos temporarily deployed to Website deployment August 26, 2023 22:23 — with GitHub Actions Inactive
@github-actions github-actions bot added A-Project Area: project A-Website Area: website labels Aug 26, 2023
@Conaclos Conaclos marked this pull request as ready for review August 26, 2023 22:24
@Conaclos Conaclos requested a review from ematipico August 26, 2023 22:24
@Conaclos Conaclos added A-Changelog Area: changelog and removed A-Changelog Area: changelog labels Aug 26, 2023
@Conaclos Conaclos force-pushed the conaclos/lint/useTemplate/rome4713 branch from 55b228c to 200291e Compare August 26, 2023 22:47
@Conaclos Conaclos temporarily deployed to Website deployment August 26, 2023 22:47 — with GitHub Actions Inactive
@Conaclos Conaclos force-pushed the conaclos/lint/useTemplate/rome4713 branch from 200291e to b6a3bf9 Compare August 27, 2023 11:02
@Conaclos Conaclos temporarily deployed to Website deployment August 27, 2023 11:02 — with GitHub Actions Inactive
@Conaclos
Copy link
Member Author

I don't understand why I got this error on CI.

Generating the rule documentation on local doesn't trigger this error (I tried a second time removing first my target directory).

Note that I added DOLLAR_CURLY to the token kind -> string mapping.
Thus, the doc generator seems to depend on a previous version of rome_js_syntax.
Is this even possible?

@ematipico
Copy link
Member

ematipico commented Aug 28, 2023

Note that I added DOLLAR_CURLY to the token kind -> string mapping. Thus, the doc generator seems to depend on a previous version of rome_js_syntax. Is this even possible?

That's a generated file; it's inside a generated/ directory. If you want to add that change, you need to modify the the source of that generated code, here:

punct: &[
(";", "SEMICOLON"),
(",", "COMMA"),
("(", "L_PAREN"),
(")", "R_PAREN"),
("{", "L_CURLY"),
("}", "R_CURLY"),
("[", "L_BRACK"),
("]", "R_BRACK"),
("<", "L_ANGLE"),
(">", "R_ANGLE"),
("~", "TILDE"),
("?", "QUESTION"),
("??", "QUESTION2"),
// These are *not* question AND dot tokens, they are one
// to distinguish between `? .3134` and `?.` per ecma specs
("?.", "QUESTIONDOT"),
("&", "AMP"),
("|", "PIPE"),
("+", "PLUS"),
("++", "PLUS2"),
("*", "STAR"),
("**", "STAR2"),
("/", "SLASH"),
("^", "CARET"),
("%", "PERCENT"),
(".", "DOT"),
("...", "DOT3"),
(":", "COLON"),
("=", "EQ"),
("==", "EQ2"),
("===", "EQ3"),
("=>", "FAT_ARROW"),
("!", "BANG"),
("!=", "NEQ"),
("!==", "NEQ2"),
("-", "MINUS"),
("--", "MINUS2"),
("<=", "LTEQ"),
(">=", "GTEQ"),
("+=", "PLUSEQ"),
("-=", "MINUSEQ"),
("|=", "PIPEEQ"),
("&=", "AMPEQ"),
("^=", "CARETEQ"),
("/=", "SLASHEQ"),
("*=", "STAREQ"),
("%=", "PERCENTEQ"),
("&&", "AMP2"),
("||", "PIPE2"),
("<<", "SHL"),
(">>", "SHR"),
(">>>", "USHR"),
("<<=", "SHLEQ"),
(">>=", "SHREQ"),
(">>>=", "USHREQ"),
("&&=", "AMP2EQ"),
("||=", "PIPE2EQ"),
("**=", "STAR2EQ"),
("??=", "QUESTION2EQ"),
("@", "AT"),
("`", "BACKTICK"),
],

@Conaclos Conaclos force-pushed the conaclos/lint/useTemplate/rome4713 branch from b6a3bf9 to ae0050b Compare August 28, 2023 16:26
@Conaclos Conaclos temporarily deployed to Website deployment August 28, 2023 16:27 — with GitHub Actions Inactive
@github-actions github-actions bot added the A-Tooling Area: internal tools label Aug 28, 2023
@Conaclos Conaclos force-pushed the conaclos/lint/useTemplate/rome4713 branch from ae0050b to 8f2a1b8 Compare August 28, 2023 16:47
@Conaclos Conaclos temporarily deployed to Website deployment August 28, 2023 16:47 — with GitHub Actions Inactive
@github-actions github-actions bot removed the A-Parser Area: parser label Aug 28, 2023
@Conaclos Conaclos force-pushed the conaclos/lint/useTemplate/rome4713 branch from 8f2a1b8 to 846e764 Compare August 28, 2023 16:50
@Conaclos Conaclos temporarily deployed to Website deployment August 28, 2023 16:50 — with GitHub Actions Inactive
@Conaclos Conaclos force-pushed the conaclos/lint/useTemplate/rome4713 branch from 846e764 to 840c489 Compare August 28, 2023 17:30
@Conaclos Conaclos temporarily deployed to Website deployment August 28, 2023 17:30 — with GitHub Actions Inactive
@github-actions github-actions bot removed the A-Tooling Area: internal tools label Aug 28, 2023
@Conaclos Conaclos temporarily deployed to Website deployment August 28, 2023 21:11 — with GitHub Actions Inactive
@github-actions github-actions bot removed the A-Project Area: project label Aug 28, 2023
@Conaclos Conaclos force-pushed the conaclos/lint/useTemplate/rome4713 branch from 0ef4008 to 840c489 Compare August 28, 2023 21:21
@Conaclos Conaclos temporarily deployed to Website deployment August 28, 2023 21:21 — with GitHub Actions Inactive
@github-actions github-actions bot added the A-Project Area: project label Aug 28, 2023
@Conaclos Conaclos force-pushed the conaclos/lint/useTemplate/rome4713 branch from 840c489 to 982ac70 Compare August 28, 2023 22:00
@Conaclos Conaclos temporarily deployed to Website deployment August 28, 2023 22:00 — with GitHub Actions Inactive
@Conaclos Conaclos force-pushed the conaclos/lint/useTemplate/rome4713 branch from 982ac70 to 350425c Compare August 29, 2023 20:05
@Conaclos Conaclos temporarily deployed to Website deployment August 29, 2023 20:05 — with GitHub Actions Inactive
@Conaclos Conaclos force-pushed the conaclos/lint/useTemplate/rome4713 branch from 350425c to 1d71bc9 Compare August 29, 2023 20:06
@Conaclos Conaclos temporarily deployed to Website deployment August 29, 2023 20:06 — with GitHub Actions Inactive
@github-actions github-actions bot added A-Changelog Area: changelog labels Aug 29, 2023
@Conaclos Conaclos force-pushed the conaclos/lint/useTemplate/rome4713 branch from 1d71bc9 to e5a5f8e Compare August 30, 2023 10:18
@Conaclos Conaclos temporarily deployed to Website deployment August 30, 2023 10:18 — with GitHub Actions Inactive
@Conaclos Conaclos force-pushed the conaclos/lint/useTemplate/rome4713 branch from e5a5f8e to c2ca25c Compare August 30, 2023 10:21
@Conaclos Conaclos temporarily deployed to Website deployment August 30, 2023 10:21 — with GitHub Actions Inactive
@Conaclos Conaclos merged commit 8f85469 into main Aug 30, 2023
@Conaclos Conaclos deleted the conaclos/lint/useTemplate/rome4713 branch August 30, 2023 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-CLI Area: CLI A-Linter Area: linter A-Project Area: project A-Website Area: website L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 lint/style/useTemplate autofix breaks for adding numbers then strings
2 participants