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(rosetta): prints colors to log file #3953

Merged
merged 5 commits into from
Feb 28, 2023
Merged

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Feb 6, 2023

Rosetta error messages contain colors codes in the errors, which makes the errors produced by Rosetta look like garbage and very hard to parse.

Strip the colors if we detect a non-TTY input.

(Copied the regex from an MIT-licensed package to avoid adding extra dependencies)


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Rosetta error messages contain colors codes in the errors, which makes
the errors produced by Rosetta look like garbage and very hard to parse.

Strip the colors if we detect a non-TTY input.

(Copied the regex from an MIT-licensed package to avoid adding extra
dependencies)
@rix0rrr rix0rrr requested a review from a team February 6, 2023 15:11
@rix0rrr rix0rrr self-assigned this Feb 6, 2023
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Feb 6, 2023
Comment on lines +247 to +257
const ANSI_PATTERN = new RegExp(
[
'[\\u001B\\u009B][[\\]()#;?]*(?:(?:(?:(?:;[-a-zA-Z\\d\\/#&.:=?%@~_]+)*|[a-zA-Z\\d]+(?:;[-a-zA-Z\\d\\/#&.:=?%@~_]*)*)?\\u0007)',
'(?:(?:\\d{1,4}(?:;\\d{0,4})*)?[\\dA-PR-TZcf-nq-uy=><~]))',
].join('|'),
'g',
);

function stripColorCodes(x: string) {
return x.replace(ANSI_PATTERN, '');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

(Copied the regex from an MIT-licensed package to avoid adding extra dependencies)

Still, it would not hurt to attribute to the original package/authors?

@RomainMuller RomainMuller added the pr/do-not-merge This PR should not be merged at this time. label Feb 14, 2023
@RomainMuller
Copy link
Contributor

Added do-not-merge as I'd still like the attribution to be added...

@rix0rrr rix0rrr removed the pr/do-not-merge This PR should not be merged at this time. label Feb 28, 2023
@mergify
Copy link
Contributor

mergify bot commented Feb 28, 2023

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Feb 28, 2023
@mergify
Copy link
Contributor

mergify bot commented Feb 28, 2023

Merging (with squash)...

@mergify
Copy link
Contributor

mergify bot commented Feb 28, 2023

Merging (with squash)...

@mergify mergify bot merged commit 0fc9229 into main Feb 28, 2023
@mergify mergify bot deleted the huijbers/rosetta-colors branch February 28, 2023 11:40
@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Feb 28, 2023
rix0rrr added a commit to aws/jsii-rosetta that referenced this pull request Apr 12, 2023
Rosetta error messages contain colors codes in the errors, which makes the errors produced by Rosetta look like garbage and very hard to parse.

Strip the colors if we detect a non-TTY input.

(Copied the regex from an MIT-licensed package to avoid adding extra dependencies)

Port of aws/jsii#3953
aws-cdk-automation pushed a commit to aws/jsii-rosetta that referenced this pull request Apr 12, 2023
Rosetta error messages contain colors codes in the errors, which makes
the errors produced by Rosetta look like garbage and very hard to parse.

Strip the colors if we detect a non-TTY input.

(Copied the regex from an MIT-licensed package to avoid adding extra
dependencies)

Port of aws/jsii#3953



---

By submitting this pull request, I confirm that my contribution is made
under the terms of the [Apache 2.0 license].

[Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants