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

fix: avoid ansi escape codes in non-tty and no color output #1655

Merged
1 commit merged into from
Feb 26, 2021

Conversation

ghost
Copy link

@ghost ghost commented Feb 22, 2021

  • Ready for review
  • Follows CONTRIBUTING rules
  • Reviewed by Snyk internal team

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 of chalk so it's not added as a direct dependency, but as best practice, maybe it should be as we use it directly?

@ghost ghost requested review from a team as code owners February 22, 2021 17:27
@ghost ghost force-pushed the smoke/fix-ascii-help branch 2 times, most recently from e13299d to 695af9e Compare February 22, 2021 17:36
Copy link
Contributor

@maxjeffos maxjeffos left a 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?


const DEFAULT_HELP = 'snyk';

const readHelpFile = (filename: string): string => {
Copy link
Contributor

@maxjeffos maxjeffos Feb 22, 2021

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)

Copy link
Author

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.

@ghost
Copy link
Author

ghost commented Feb 22, 2021

Is there a reason you didn't use at least part of the PR description template?

I asked and was told it was fine. I agree though; the why is important. I'll polish this up tomorrow. 👍

@JackuB
Copy link
Contributor

JackuB commented Feb 23, 2021

Ha, my bad, told @jahed-snyk to not fill it :)

@ghost ghost force-pushed the smoke/fix-ascii-help branch from 695af9e to 2a9eea9 Compare February 24, 2021 16:06
@github-actions
Copy link
Contributor

github-actions bot commented Feb 24, 2021

Expected release notes (by @jahed-snyk)

fixes:
avoid ansi escape codes in non-tty and no color output (2a9eea9)

  • I hereby acknowledge these release notes are 🥙 AWESOME 🥙

@ghost ghost changed the title fix: avoid ascii codes in non-tty and no color output fix: avoid ansi escape codes in non-tty and no color output Feb 24, 2021
@ghost
Copy link
Author

ghost commented Feb 24, 2021

Updated OP with details now. Also added a question:

strip-ansi is a part of chalk so it's not added as a direct dependency, but as best practice, maybe it should be as we use it directly?

@ghost ghost merged commit 3a2a30e into master Feb 26, 2021
@ghost ghost deleted the smoke/fix-ascii-help branch February 26, 2021 16:20
@ghost
Copy link
Author

ghost commented Feb 26, 2021

Merged. To answer the remaining question, we already have strip-ansi in the package.json. 🙃

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants