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

feat(rome_cli): add a --force-colors argument #3625

Merged
merged 2 commits into from
Nov 14, 2022
Merged

Conversation

leops
Copy link
Contributor

@leops leops commented Nov 9, 2022

Summary

Fixes #3623

This adds a new --force-colors argument to the CLI to force the EnvConsole to use ANSI colors event if the stream would have otherwise been determined to be incompatible (not a TTY)

Test Plan

I've added a few tests that ensure the --no-colors and --force-colors arguments are correctly parsed, unfortunately it's not really possible to check they actually work since this relies on using the actual EnvConsole instead of the MemoryConsole and writes directly to the stdout / stderr streams using termcolor, meaning it's not possible to inspect the output that got written by the CLI run (this also means the tests also circumvent the println! inhibiting behavior of the Rust test framework and write directly to the test output anyway)

@leops leops requested a review from xunilrj as a code owner November 9, 2022 15:59
@leops leops requested a review from a team November 9, 2022 15:59
@netlify
Copy link

netlify bot commented Nov 9, 2022

Deploy Preview for docs-rometools ready!

Name Link
🔨 Latest commit dc5ca27
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/637243236fc042000aef571b
😎 Deploy Preview https://deploy-preview-3625--docs-rometools.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@leops leops temporarily deployed to netlify-playground November 9, 2022 15:59 Inactive
@github-actions
Copy link

github-actions bot commented Nov 9, 2022

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 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 12397 12397 0
Failed 3860 3860 0
Panics 0 0 0
Coverage 76.26% 76.26% 0.00%

@github-actions
Copy link

github-actions bot commented Nov 9, 2022

@MichaReiser
Copy link
Contributor

Nice! What's your motivation behind introducing a new option rather than merging --no-colors and --force-colors into --color=false and --color?

@leops
Copy link
Contributor Author

leops commented Nov 9, 2022

Nice! What's your motivation behind introducing a new option rather than merging --no-colors and --force-colors into --color=false and --color?

I think --color doesn't really communicate the argument is forcing the use of ANSI colors even if the output stream is incompatible, which may lead to an unreadable output stream if used incorrectly. Alternatively it would have been possible to expose this setting as --color=off and --color=force respectively to keep it as a single argument name on the CLI

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.

I'll leave the decision on having two options vs a single color option to you.

Either way, please hold back with merging until after the 10.0.1 release as this is a new functionality according to semver

@leops leops added this to the 11.0.0 milestone Nov 9, 2022
@leops leops added the A-CLI Area: CLI label Nov 10, 2022
Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Where do we plan to document the combination of --no-colors and --force-colors and how they work together? Having one options allows for better documentation, but two options requires better documentation (especially in the rome --help command) to explain how they work together.

@leops leops requested a review from a team November 14, 2022 13:29
@leops leops merged commit 58fbb93 into main Nov 14, 2022
@leops leops deleted the feature/force-colors branch November 14, 2022 16:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

🐛 Missing colors when rome is spawned from node
3 participants