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

refactor(rome_formatter): new format API, part 2 #2038

Merged
merged 5 commits into from
Feb 2, 2022

Conversation

ematipico
Copy link
Contributor

@ematipico ematipico commented Feb 2, 2022

Summary

Part of #1996

This PR refactors more code to use the new format API.

While doing so, I encountered new edge cases, because of that I created new APIs for these cases:

  • try_format_with_or which allow the use of the try operator inside the closures
  • try_format_with which allow the use of the try operator inside the closures

I've also fixed some bug in the process.

Test Plan

Existing tests should not break

@github-actions
Copy link

github-actions bot commented Feb 2, 2022

Parser conformance results on ubuntu-latest

T262

Test result main count This PR count Difference
Total 45250 45250 0
Passed 44130 44130 0
Failed 1120 1120 0
Panics 0 0 0
Coverage 97.52% 97.52% 0.00%

TS

Test result main count This PR count Difference
Total 15976 15976 0
Passed 11126 11126 0
Failed 4848 4848 0
Panics 2 2 0
Coverage 69.64% 69.64% 0.00%

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Feb 2, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 29f7641
Status: ✅  Deploy successful!
Preview URL: https://b44bc198.tools-8rn.pages.dev

View logs

@@ -14,6 +14,8 @@ class Foo extends Boar {

}

* generator (){}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This case was uncovered by our formatter and it was buggy. The new trait system worked quite well.

@ematipico
Copy link
Contributor Author

!bench_formatter

@github-actions
Copy link

github-actions bot commented Feb 2, 2022

Bench results on ubuntu-latest

group    main    pr
-----    ----    --
formatter
/compiler.js    1.00    298.9±0.89ms     3.5 MB/sec    1.00    299.4±1.14ms     3.5 MB/sec
formatter
/d3.min.js    1.00    209.7±1.09ms  1280.0 KB/sec    1.01    211.3±1.52ms  1270.4 KB/sec
formatter
/dojo.js    1.00     16.1±0.09ms     4.3 MB/sec    1.01     16.2±0.13ms     4.2 MB/sec
formatter
/jquery.min.js    1.00     61.4±0.44ms  1379.3 KB/sec    1.00     61.4±0.33ms  1378.0 KB/sec
formatter
/pixi.min.js    1.00    248.1±2.47ms  1811.5 KB/sec    1.00    248.7±0.95ms  1807.0 KB/sec
formatter
/react-dom.production.min.js    1.00     82.9±0.35ms  1421.1 KB/sec    1.01     83.7±0.53ms  1408.5 KB/sec
formatter
/react.production.min.js    1.00      3.5±0.00ms  1784.3 KB/sec    1.01      3.6±0.02ms  1772.9 KB/sec
formatter
/tex-chtml-full.js    1.00    656.6±1.75ms  1421.2 KB/sec    1.00    659.5±1.82ms  1414.9 KB/sec
formatter
/three.min.js    1.00    296.3±1.80ms  2029.3 KB/sec    1.00    297.4±1.37ms  2021.3 KB/sec
formatter
/vue.global.prod.js    1.00    101.9±0.50ms  1211.1 KB/sec    1.01    102.7±0.90ms  1201.7 KB/sec

Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

@ematipico the benchmark output is no longer printed as a table which makes it hard to read the results. Would you mind having a look?

@ematipico
Copy link
Contributor Author

ematipico commented Feb 2, 2022

@ematipico the benchmark output is no longer printed as a table which makes it hard to read the results. Would you mind having a look?

It was never printed as a markdown table (check the code), but just as simple block and a mix of tabs/spaces to make the formatting decent. I can change it make a markdown table if you want

@MichaReiser
Copy link
Contributor

@ematipico the benchmark output is no longer printed as a table which makes it hard to read the results. Would you mind having a look?

It was never printed as a markdown table (check the code), but just as simple block and a mix of tabs/spaces to make the formatting decent. I can change it make a markdown table if you want

You're right. It also prints somewhat misaligned locally. I wonder why that is. Maybe there's some way to get the nice formatting back on console & PR comment.

@ematipico ematipico requested a review from leops February 2, 2022 16:20
@ematipico
Copy link
Contributor Author

@ematipico the benchmark output is no longer printed as a table which makes it hard to read the results. Would you mind having a look?

It was never printed as a markdown table (check the code), but just as simple block and a mix of tabs/spaces to make the formatting decent. I can change it make a markdown table if you want

You're right. It also prints somewhat misaligned locally. I wonder why that is. Maybe there's some way to get the nice formatting back on console & PR comment.

The output comes form critcmp, which is installed and run only during the CI. Unfortunately I don't see any markdown option. I guess the only solution would be to build our own.

@ematipico ematipico merged commit 28ccf2e into main Feb 2, 2022
@ematipico ematipico deleted the refactor/new-format-syntax-part-2 branch February 2, 2022 16:25
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