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

feat(rome_js_analyze): useTemplate #2803

Merged
merged 26 commits into from
Jul 19, 2022
Merged

feat(rome_js_analyze): useTemplate #2803

merged 26 commits into from
Jul 19, 2022

Conversation

IWANABETHATGUY
Copy link
Contributor

@IWANABETHATGUY IWANABETHATGUY commented Jul 1, 2022 •
edited
Loading

Summary

  1. Resolved useTemplate #2750

Test Plan

  1. cargo test

@IWANABETHATGUY IWANABETHATGUY changed the title feat: 🎸 init feat(rome_js_analyze): useTemplate Jul 1, 2022
@IWANABETHATGUY IWANABETHATGUY marked this pull request as ready for review July 5, 2022 06:19
IWANABETHATGUY
IWANABETHATGUY commented Jul 5, 2022
View reviewed changes
@IWANABETHATGUY
Copy link
Contributor Author

@ematipico , @leops , Would you mind helping with reviewing

leops
leops reviewed Jul 5, 2022
View reviewed changes
@IWANABETHATGUY IWANABETHATGUY requested a review from leops July 5, 2022 15:44
ematipico
ematipico reviewed Jul 6, 2022
View reviewed changes
leops
leops reviewed Jul 7, 2022
View reviewed changes
@IWANABETHATGUY IWANABETHATGUY requested review from ematipico and leops July 8, 2022 06:24
@IWANABETHATGUY
Copy link
Contributor Author

@ematipico , I guess this pull request is ready to merge, I have addressed all issues. Would you mind having a look?

ematipico
ematipico suggested changes Jul 8, 2022
View reviewed changes
Copy link
Contributor

@ematipico ematipico left a comment •
edited
Loading

Choose a reason for hiding this comment

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

A couple of things before merging:

  1. could you rebase your fork? we fixed the CI failures and it should be fine now;
  2. could you add invalid cases where strings have spaces? Two/three should be fine I think (with some nested case)

An example:

0;
foo() + " bar"; // before
`${foo()} bar`; // after

@IWANABETHATGUY
Copy link
Contributor Author

I have rebased the main branch and added an extra test case.

@IWANABETHATGUY IWANABETHATGUY requested a review from ematipico July 9, 2022 07:33
@IWANABETHATGUY IWANABETHATGUY requested a review from xunilrj as a code owner July 11, 2022 07:59
@IWANABETHATGUY IWANABETHATGUY requested a review from a team July 11, 2022 07:59
@IWANABETHATGUY
Copy link
Contributor Author

@xunilrj would you mind helping review this pr?

xunilrj
xunilrj reviewed Jul 14, 2022
View reviewed changes
xunilrj
xunilrj reviewed Jul 14, 2022
View reviewed changes
xunilrj
xunilrj reviewed Jul 14, 2022
View reviewed changes
crates/rome_js_analyze/src/analyzers/js/use_template.rs
reduced_exprs.extend(flatten_template_element_list(template.elements())?);
}
_ => {
// drop the trivia of original expression make the generated `JsTemplate` a little nicer, if we don't do this
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we drop trivia if they are comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should keep the comments.

xunilrj
xunilrj reviewed Jul 14, 2022
View reviewed changes
xunilrj
xunilrj reviewed Jul 14, 2022
View reviewed changes
xunilrj
xunilrj reviewed Jul 14, 2022
View reviewed changes
xunilrj
xunilrj reviewed Jul 14, 2022
View reviewed changes
IWANABETHATGUY and others added 21 commits July 19, 2022 10:13
@IWANABETHATGUY
Copy link
Contributor Author

!bench_analyzer

@github-actions
Copy link

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.00      3.5±0.01ms     3.3 MB/sec    1.12      4.0±0.02ms     2.9 MB/sec
analyzer/index.js         1.00      8.4±0.01ms     3.8 MB/sec    1.01      8.4±0.08ms     3.8 MB/sec
analyzer/lint.ts          1.00      5.8±0.01ms     7.2 MB/sec    1.00      5.8±0.02ms     7.2 MB/sec
analyzer/parser.ts        1.00     13.0±0.07ms     3.7 MB/sec    1.01     13.0±0.06ms     3.7 MB/sec
analyzer/router.ts        1.00      8.3±0.01ms     7.2 MB/sec    1.00      8.3±0.06ms     7.2 MB/sec
analyzer/statement.ts     1.01     11.1±0.05ms     3.2 MB/sec    1.00     11.0±0.10ms     3.2 MB/sec
analyzer/typescript.ts    1.00     16.2±0.07ms     3.4 MB/sec    1.00     16.2±0.09ms     3.4 MB/sec

@ematipico ematipico merged commit 5818929 into rome:main Jul 19, 2022
@IWANABETHATGUY IWANABETHATGUY deleted the feat/use-template branch July 19, 2022 08:13
@IWANABETHATGUY IWANABETHATGUY mentioned this pull request Jul 19, 2022
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@xunilrj xunilrj xunilrj left review comments

@ematipico ematipico ematipico approved these changes

@leops leops leops approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

useTemplate

4 participants

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