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

feat(rome_js_syntax): better api for picking jsx attributes #3458

Merged
merged 8 commits into from
Oct 21, 2022

Conversation

xunilrj
Copy link
Contributor

@xunilrj xunilrj commented Oct 19, 2022

Summary

This PR try to simplify the API for retrieving JSX attributes.
Unfortunately, to achieve this it needs to use unsafe, because JsxAttribute is not Copy.

let [a, c, d] = list.find_attributes_by_name(["a", "c", "d"]);

Test Plan

cargo test -p rome_js_analyze -- find_attributes_by_name 

@xunilrj xunilrj requested a review from leops as a code owner October 19, 2022 17:27
@xunilrj xunilrj requested a review from a team October 19, 2022 17:27
@netlify
Copy link

netlify bot commented Oct 19, 2022

Deploy Preview for rometools canceled.

Name Link
🔨 Latest commit 55c7cae
🔍 Latest deploy log https://app.netlify.com/sites/rometools/deploys/6352a529e308860008bbb364

@xunilrj xunilrj temporarily deployed to netlify-playground October 19, 2022 17:27 Inactive
@github-actions
Copy link

Parser conformance results on ubuntu-latest

js/262

Test result main count This PR count Difference
Total 45879 45879 0
Passed 44939 44939 0
Failed 940 940 0
Panics 0 0 0
Coverage 97.95% 97.95% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 39 39 0
Passed 36 36 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.31% 92.31% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 5946 5946 0
Passed 1621 1621 0
Failed 4325 4325 0
Panics 0 0 0
Coverage 27.26% 27.26% 0.00%

ts/babel

Test result main count This PR count Difference
Total 588 588 0
Passed 519 519 0
Failed 69 69 0
Panics 0 0 0
Coverage 88.27% 88.27% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 16257 16257 0
Passed 12395 12395 0
Failed 3862 3862 0
Panics 0 0 0
Coverage 76.24% 76.24% 0.00%

@github-actions
Copy link

github-actions bot commented Oct 19, 2022

@xunilrj xunilrj force-pushed the feature/better-api-for-jsx-attributes branch from 0a373bb to 81ab611 Compare October 19, 2022 19:17
@xunilrj xunilrj temporarily deployed to netlify-playground October 19, 2022 19:17 Inactive
@xunilrj xunilrj temporarily deployed to netlify-playground October 20, 2022 16:47 Inactive
@xunilrj xunilrj temporarily deployed to netlify-playground October 20, 2022 21:39 Inactive
@xunilrj xunilrj temporarily deployed to netlify-playground October 20, 2022 21:44 Inactive
) -> [Option<JsxAttribute>; N] {
// assert there are no duplicates
debug_assert!(HashSet::<_>::from_iter(names_to_lookup).len() == N);
debug_assert!(N <= 16);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it necessary that N is smaller than 16?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to put a safeguard, given that everything is allocated on the stack.

Copy link
Contributor

@ematipico ematipico Oct 21, 2022

Choose a reason for hiding this comment

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

Would you mind add a comment to explain why? I personally don't know why that is safe and against what. That would be appreciated

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.

@xunilrj xunilrj temporarily deployed to netlify-playground October 21, 2022 10:38 Inactive
@xunilrj xunilrj temporarily deployed to netlify-playground October 21, 2022 11:00 Inactive
@xunilrj xunilrj temporarily deployed to netlify-playground October 21, 2022 13:32 Inactive
@xunilrj xunilrj force-pushed the feature/better-api-for-jsx-attributes branch from 7fceed3 to 9967517 Compare October 21, 2022 13:37
@xunilrj xunilrj temporarily deployed to netlify-playground October 21, 2022 13:37 Inactive
@xunilrj xunilrj force-pushed the feature/better-api-for-jsx-attributes branch from 9967517 to 28c86f4 Compare October 21, 2022 13:40
@xunilrj xunilrj temporarily deployed to netlify-playground October 21, 2022 13:40 Inactive
@xunilrj xunilrj temporarily deployed to netlify-playground October 21, 2022 13:57 Inactive
@xunilrj xunilrj merged commit 51c4502 into main Oct 21, 2022
@xunilrj xunilrj deleted the feature/better-api-for-jsx-attributes branch October 21, 2022 14:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants