-
Notifications
You must be signed in to change notification settings - Fork 552
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
fix: avoid ansi escape codes in non-tty and no color output #1655
Conversation
e13299d
to
695af9e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description could use some more info, too (like "why"). Is there a reason you didn't use at least part of the PR description template?
src/cli/commands/help.ts
Outdated
|
||
const DEFAULT_HELP = 'snyk'; | ||
|
||
const readHelpFile = (filename: string): string => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think we have a policy on this, but this kind of function definition is not particularly helpful unless it is required (i.e. when using as a closure) and does not appear to be inline with the norm. (not blocking)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed it to use plain old functions.
I asked and was told it was fine. I agree though; the why is important. I'll polish this up tomorrow. 👍 |
Ha, my bad, told @jahed-snyk to not fill it :) |
695af9e
to
2a9eea9
Compare
Expected release notes (by @jahed-snyk) fixes:
|
Updated OP with details now. Also added a question:
|
Merged. To answer the remaining question, we already have strip-ansi in the package.json. 🙃 |
What does this PR do?
When the terminal doesn't support colour (either NO_COLOR env variable or not TTY as detected by NodeJS), it will remove every ANSI escape code when printing the
help
output. This avoids printing them in plain text and obfuscating the help text.Where should the reviewer start?
Only a few files to check.
How should this be manually tested?
Run
snyk help | cat
will print ANSI codes in plain text before this change. After it should print plain unformatted text.Any background context you want to provide?
Originally noticed in some Powershell sessions.
What are the relevant tickets?
None.
Screenshots
None.
Additional questions
strip-ansi
is a part ofchalk
so it's not added as a direct dependency, but as best practice, maybe it should be as we use it directly?