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

Add missing escape sequences to ConvertAnsi plugin #4544

Merged
merged 2 commits into from
Sep 27, 2017

Conversation

pedrottimark
Copy link
Contributor

Summary

  • Because colors share same close sequences, some closing tags didn’t match opening tags
  • Without close sequences (which are shared, by the way) for bold and dim their opening tags didn’t match closing tags

The mismatches confused me a bit in the beginning.
Adding the missing cyan theme color confused us when we reviewed #3429

Additional colors:

  • gray (also known as bright black) in default theme for pretty-format plugin
  • yellow in default theme for pretty-format plugin also in jest-diff
  • white in jest-cli

Additional modifier:

  • inverse is used in jest-cli also will be useful in jest-diff and jest-matcher-utils

Additional close sequences:

  • bold
  • dim

Residue: While reviewing the snapshots, a few mismatched </> confused me because reset.open and reset.close share the same escape sequence.

Test plan

Ouch: 511 snapshots updated in 15 test suites

@codecov-io
Copy link

Codecov Report

Merging #4544 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4544      +/-   ##
==========================================
+ Coverage   56.01%   56.04%   +0.02%     
==========================================
  Files         185      185              
  Lines        6264     6268       +4     
  Branches        3        3              
==========================================
+ Hits         3509     3513       +4     
  Misses       2754     2754              
  Partials        1        1
Impacted Files Coverage Δ
packages/pretty-format/src/plugins/convert_ansi.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e3c423...4d25d9a. Read the comment docs.

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

I'm OK with the change.
ANSI colors don't need to have a closing tag, they just begin color sequence and override each other, although chalk is nice enough to close them.

@cpojer cpojer merged commit 15b084d into jestjs:master Sep 27, 2017
@pedrottimark pedrottimark deleted the convert-ansi-sequences branch September 27, 2017 16:14
tabrindle pushed a commit to tabrindle/jest that referenced this pull request Oct 2, 2017
* Add missing escape sequences to ConvertAnsi plugin

* Correct opening tags for gray and yellow
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants