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

feat(rome_cli): expose the --verbose flag to the CLI #3812

Merged
merged 4 commits into from
Nov 23, 2022
Merged

Conversation

leops
Copy link
Contributor

@leops leops commented Nov 21, 2022

Summary

Verbose advices on diagnostics are currently unused, as they always get printed when the diagnostic is displayed with PrintDiagnostic. This PR adds a second argument to the diagnostics printer to configure whether verbose advices should be printed, and exposes this setting as the --verbose flag on the relevant CLI commands (check, ci and format).

Test Plan

I've added a few tests to ensure the CLI correctly accepts the new argument. Since we don't have any diagnostic making use of verbose advices yet it doesn't really make a difference in the output though.

@leops leops added A-CLI Area: CLI A-Diagnostic Area: errors and diagnostics labels Nov 21, 2022
Base automatically changed from refactor/remove-legacy-diagnostics to main November 21, 2022 11:15
@netlify
Copy link

netlify bot commented Nov 21, 2022

Deploy Preview for docs-rometools canceled.

Name Link
🔨 Latest commit 12f40b3
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/637e29553759ed00082c9c41

@leops leops marked this pull request as ready for review November 21, 2022 11:20
@leops leops requested a review from a team November 21, 2022 11:20
@calibre-analytics
Copy link

calibre-analytics bot commented Nov 21, 2022

Comparing feat(rome_cli): expose the --verbose flag to the CLI Snapshot #4 to median since last deploy of rome.tools.

LCP? CLS? TBT?
Overall
Median across all pages and test profiles
2.5s
from 264ms
0.0
no change
209ms
no change
Chrome Desktop
Chrome Desktop • Cable
2.5s
from 264ms
0.0
no change
336ms
from 22ms
iPhone, 4G LTE
iPhone 12 • 4G LTE
1.09s
from 238ms
0.0
no change
4ms
no change
Motorola Moto G Power, 3G connection
Motorola Moto G Power • Regular 3G
16.9s
from 1.07s
0.0
no change
209ms
no change

1 page tested

 Home

Browser previews

Chrome Desktop iPhone, 4G LTE Motorola Moto G Power, 3G connection
Chrome Desktop iPhone, 4G LTE Motorola Moto G Power, 3G connection

Most significant changes

Value Budget
JS Parse & Compile
Motorola Moto G Power, 3G connection
2.02s
from 27ms
Total JavaScript Size in Bytes
Chrome Desktop
5.35 MB
from 86.8 KB
Total JavaScript Size in Bytes
iPhone, 4G LTE
5.35 MB
from 86.8 KB
Total JavaScript Size in Bytes
Motorola Moto G Power, 3G connection
5.35 MB
from 86.8 KB
JS Parse & Compile
iPhone, 4G LTE
472ms
from 10ms

27 other significant changes: JS Parse & Compile on Chrome Desktop, Largest Contentful Paint on Motorola Moto G Power, 3G connection, First Contentful Paint on Motorola Moto G Power, 3G connection, Total Blocking Time on Chrome Desktop, Time to Interactive on Motorola Moto G Power, 3G connection, Total CSS Size in Bytes on Chrome Desktop, Total CSS Size in Bytes on iPhone, 4G LTE, Total CSS Size in Bytes on Motorola Moto G Power, 3G connection, Total Page Size in Bytes on Chrome Desktop, Total Page Size in Bytes on iPhone, 4G LTE, Total Page Size in Bytes on Motorola Moto G Power, 3G connection, Time to Interactive on Chrome Desktop, First Contentful Paint on Chrome Desktop, Largest Contentful Paint on Chrome Desktop, Number of Requests on Motorola Moto G Power, 3G connection, Number of Requests on Chrome Desktop, Number of Requests on iPhone, 4G LTE, Speed Index on Motorola Moto G Power, 3G connection, Time to Interactive on iPhone, 4G LTE, Largest Contentful Paint on iPhone, 4G LTE, First Contentful Paint on iPhone, 4G LTE, Speed Index on Chrome Desktop, Total HTML Size in Bytes on Chrome Desktop, Total HTML Size in Bytes on iPhone, 4G LTE, Total HTML Size in Bytes on Motorola Moto G Power, 3G connection, Lighthouse Performance Score on Motorola Moto G Power, 3G connection, Lighthouse Performance Score on Chrome Desktop

Calibre: Site dashboard | View this PR | Edit settings | View documentation

@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 44936 44936 0
Failed 943 943 0
Panics 0 0 0
Coverage 97.94% 97.94% 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 1757 1757 0
Failed 4189 4189 0
Panics 0 0 0
Coverage 29.55% 29.55% 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 12397 12397 0
Failed 3860 3860 0
Panics 0 0 0
Coverage 76.26% 76.26% 0.00%

@@ -37,6 +37,7 @@ const CHECK: Markup = markup! {
"<Dim>"--apply"</Dim>" Apply safe fixes
"<Dim>"--apply-suggested"</Dim>" Apply safe and suggested fixes
"<Dim>"--max-diagnostics"</Dim>" Cap the amount of diagnostics displayed (default: 20)
"<Dim>"--verbose"</Dim>" Print additional verbose advices on diagnostics
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the --verbose argument should be global. Much like --colors.

Any command can emit diagnostics, hence any diagnostic can have, theoretically, verbose advice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well at the moment only check, ci and format can print diagnostics since Termination hasn't been migrated to the diagnostic system yet. But yes eventually I think this will become a global flag for the CLI

@@ -889,7 +889,7 @@ mod tests {
..TestDiagnostic::empty()
};

let diag = markup!({ PrintDiagnostic(&diag) }).to_owned();
let diag = markup!({ PrintDiagnostic(&diag, true) }).to_owned();
Copy link
Contributor

Choose a reason for hiding this comment

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

The true isn't very self explanatory. How about a PrintDiagnostic::verbose() and PrintDiagnostic::non_verbose()

@ematipico
Copy link
Contributor

@leops if you're able to fix the CI and merge it, I might be able to use this new feature for the suppression comments actions.

@leops leops merged commit 37d4d5b into main Nov 23, 2022
@leops leops deleted the feature/cli-verbose branch November 23, 2022 14:26
jeysal added a commit to jeysal/rometools that referenced this pull request Nov 24, 2022
* upstream/main: (73 commits)
  fix(semantic_analyzers): style/noShoutyConstants does not recognize multiple uses of a constant. (rome#3789)
  feat(rome_js_analyze): useDefaultSwitchClauseLast (rome#3791)
  chore: run rustfmt and typo fix (rome#3840)
  feat(rome_js_analyze): use exhaustive deps support properties (rome#3581)
  website(docs): Fix text formatting (rome#3828)
  feat(rome_js_analyze): `noVoidTypeReturn` (rome#3806)
  feat(rome_cli): expose the `--verbose` flag to the CLI (rome#3812)
  fix(rome_diagnostics): allow diagnostic locations to be created without a resource (rome#3834)
  feat(rome_js_analyze): add noExtraNonNullAssertion rule (rome#3797)
  fix(rome_lsp): lsp friendly catch unwind (rome#3740)
  feat(rome_js_semantic): model improvements (rome#3825)
  feat(rome_json_parser): JSON Lexer (rome#3809)
  feat(rome_js_analyze): implement `noDistractingElements` (rome#3820)
  fix(rome_js_formatter): shothanded named import line break with default import (rome#3826)
  feat(rome_js_analyze): `noConstructorReturn` (rome#3805)
  feat(website): change enabledNurseryRules to All/Recommended select (rome#3810)
  feat(rome_js_analyze): noSetterReturn
  feat(rome_js_analyze): noConstructorReturn
  feat(rome_analyze): suppress rule via code actions (rome#3572)
  feat(rome_js_analyze): `noVar` (rome#3765)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-CLI Area: CLI A-Diagnostic Area: errors and diagnostics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants