Skip to content
This repository has been archived by the owner on Feb 12, 2022. It is now read-only.

Strip ANSI characters when fitting terminal width #218

Merged
merged 1 commit into from
Apr 6, 2018

Conversation

HaraldNordgren
Copy link
Contributor

@HaraldNordgren HaraldNordgren commented Mar 17, 2018

Fixing a bug introduced in #216. I noticed that the previous fix was not taking all ANSI color sequences into account when calculating the lengths. The effect of this is that the table output will be still be aligned, but too narrow.

In the original implementation that stretched over heroku-apps and heroku-cli-util the ANSI stripping was done implicitly through https://github.com/heroku/heroku-cli-util/blob/v8.0.0/lib/table.js#L74. I forgot to include that when migrating that code to the current implementation.

Here I am creating a fix which is to call stripAnsi(formattedValue) when calculating the optimization lengths.

The rest of the PR is for increasing testability of the code. The older tests won't fail for this scenario since colors are turned off by default in the mock scenario.

@codecov
Copy link

codecov bot commented Mar 17, 2018

Codecov Report

Merging #218 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #218      +/-   ##
==========================================
+ Coverage   95.79%   95.82%   +0.03%     
==========================================
  Files          76       76              
  Lines        1570     1582      +12     
  Branches      297      298       +1     
==========================================
+ Hits         1504     1516      +12     
  Misses         63       63              
  Partials        3        3
Impacted Files Coverage Δ
src/commands/releases/index.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 83b397d...a59397a. Read the comment docs.

@HaraldNordgren HaraldNordgren force-pushed the align_releases_length branch 3 times, most recently from a85c05e to cbe9a48 Compare March 17, 2018 17:18
@HaraldNordgren HaraldNordgren changed the title Escape ANSI characters when fitting terminal width Strip ANSI characters when fitting terminal width Mar 17, 2018
@HaraldNordgren HaraldNordgren force-pushed the align_releases_length branch from cbe9a48 to 7f09cd1 Compare March 17, 2018 17:22
@HaraldNordgren HaraldNordgren force-pushed the align_releases_length branch from 7f09cd1 to f372ce6 Compare March 26, 2018 07:42
@HaraldNordgren
Copy link
Contributor Author

Ping @jdxcode @RasPhilCo @ransombriggs

1 similar comment
@HaraldNordgren
Copy link
Contributor Author

Ping @jdxcode @RasPhilCo @ransombriggs

@HaraldNordgren HaraldNordgren force-pushed the align_releases_length branch from f372ce6 to a59397a Compare April 5, 2018 20:23
@jdx jdx merged commit 12b2aee into heroku:master Apr 6, 2018
@HaraldNordgren HaraldNordgren deleted the align_releases_length branch April 7, 2018 10:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants