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

refactor(js_formatter): move NeedsParentheses trait to biome_js_syntax #3541

Merged
merged 2 commits into from
Jul 30, 2024

Conversation

Conaclos
Copy link
Member

@Conaclos Conaclos commented Jul 29, 2024

Summary

This PR moves the NeedsParentheses trait and its implementations from biome_js_formatter to biome_js_syntax.

I had also to move some utility types (AnyJsBinaryLikeExpression, AnyJsBinaryLikeLeftExpression, BinaryLikeOperator and AnyJsExpressionLeftSide).
I plan to move AnyJsExpressionLeftSide in a dedicated file.
Also, I want to evaluate converting some methods to functions in order to move them back in biome_js_formatter.

I removed some implementations of NeedsParentheses that was not used (AnyTsType) and implementations of NeedsParentheses for AnyTsType variants that return false.

I inlined some small utility functions and removed some utility types (TsAsOrSatisfiesExpression, TsAsOrSatisfiesAssignment). This reduces node cloning and improves readability of the code.
I took the opportunity of improving some implementations (replacing map_or(false, X) with is_some_and(X), use consistently cast/cast_ref/try_cast).

Many implementations of NeedsParentheses::needs_parentheses used the attribute #[inline] or #[inline(always)]. I replaced all occurrence of #[inline(always)] with #[inline] and added #[inline] where it was missing. I am not sure if it is actually useful to add this attribute. I added it for consistency,

Remaining tasks:

  • Update docs
  • Move AnyJsExpressionLeftSide in a dedicated file?

Test Plan

I first moved all tests from biome_js_formatter to biome_js_syntax; unfortunately the test assertions use the formatter.
Thus, I commented the tests in biome_js_syntax and restored the ones in biome_js_formatter.
I don't think we are able to properly test the implementations in biome_js_syntax because building nodes by hand is a lot of work and doesn't allow accessing parent nodes. Many NeedsParentheses implementations retrieve the parent node to decide if a node needs parentheses.

@github-actions github-actions bot added A-Parser Area: parser A-Formatter Area: formatter L-JavaScript Language: JavaScript and super languages labels Jul 29, 2024
@Conaclos Conaclos force-pushed the conaclos/move-needs-parentheses branch from abf82cf to b33e5f1 Compare July 29, 2024 08:56
Copy link
Contributor

github-actions bot commented Jul 29, 2024

Parser conformance results on

js/262

Test result main count This PR count Difference
Total 48452 48452 0
Passed 47254 47254 0
Failed 1198 1198 0
Panics 0 0 0
Coverage 97.53% 97.53% 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 6544 6544 0
Passed 2197 2197 0
Failed 4347 4347 0
Panics 0 0 0
Coverage 33.57% 33.57% 0.00%

ts/babel

Test result main count This PR count Difference
Total 670 670 0
Passed 598 598 0
Failed 72 72 0
Panics 0 0 0
Coverage 89.25% 89.25% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 18327 18327 0
Passed 14020 14020 0
Failed 4307 4307 0
Panics 0 0 0
Coverage 76.50% 76.50% 0.00%

@Conaclos Conaclos force-pushed the conaclos/move-needs-parentheses branch 5 times, most recently from 3482533 to 95c6d81 Compare July 29, 2024 09:11
Copy link

codspeed-hq bot commented Jul 29, 2024

CodSpeed Performance Report

Merging #3541 will not alter performance

Comparing conaclos/move-needs-parentheses (2921ae8) with main (6be2bdd)

Summary

✅ 104 untouched benchmarks

@Conaclos Conaclos changed the title refactor(js_formatter): move NeedsParentheses trait and impls to biome_js_syntax refactor(js_formatter): move NeedsParentheses trait to biome_js_syntax Jul 29, 2024
@Conaclos Conaclos force-pushed the conaclos/move-needs-parentheses branch from 95c6d81 to 2be3617 Compare July 30, 2024 14:49
@Conaclos Conaclos marked this pull request as ready for review July 30, 2024 14:49
@Conaclos Conaclos requested review from a team July 30, 2024 15:10
@Conaclos Conaclos force-pushed the conaclos/move-needs-parentheses branch from 2be3617 to 2921ae8 Compare July 30, 2024 15:28
@Conaclos Conaclos merged commit 872dd8e into main Jul 30, 2024
15 checks passed
@Conaclos Conaclos deleted the conaclos/move-needs-parentheses branch July 30, 2024 15:55
@Conaclos Conaclos added the A-Changelog Area: changelog label Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-Formatter Area: formatter A-Parser Area: parser L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants