chalk / wrap-ansi

Wordwrap a string with ANSI escape codes
MIT License
120 stars 25 forks source link

added support for 'hard' vs. 'soft' wrapping (also code-coverage). #3

Closed bcoe closed 9 years ago

bcoe commented 9 years ago

In this pull request I've added support for the concept of hard vs. soft wrapping:

soft wrapping allows words that take up more than cols to be printed on a single line.

hard wrapping does not allow a row to ever contain more than cols characters.

This functionality is modeled after @substack's node-wordwrap.

In this pull I've also added code coverage support, which helps me ensure that I'm not being lazy while writing tests.

How I Tested Hard vs. Soft Wrapping

  1. I wrote a testing script that allowed me to compare substack's wordwrap library, to the ansi wordwrap module. wordwrap is used by cliui and yargs, and my goal was to make ansi-wordwrap a drop in replacement. Here's the script I wrote:
var assert = require('assert');
var stripAnsi = require('strip-ansi');
var chalk = require('chalk');

var fixture = 'The thisisareallylongword quick brown ' + chalk.red('fox jumped over ') + 'the lazy ' + chalk.green('dog and then ran away with the unicorn.');
var wrap = require('./');
var wrap2 = require('wordwrap');

for (var cols = 1; cols < 30; cols++) {
  console.log('cols = ', cols);
  console.log(wrap(fixture, cols, {hard: true}), '\n---');
  console.log(wrap2.hard(cols)(stripAnsi(fixture)), '\n---');
  assert.equal(stripAnsi(wrap(fixture, cols, {hard: true})), wrap2.hard(cols)(stripAnsi(fixture)));
}

I used this script to compare both soft and hard wrapping between the two modules, and managed to get the output pretty close to parity (I actually think that I found a few bugs in wordwrap, that are not present in ansi-wrap related to wrapping close to column boundaries) \o/

screen shot 2015-10-04 at 2 24 08 pm

  1. my second test was to npm link, wrap-ansi and use it as a drop in replacement for wordwrap. Both the yargs and cliui codebases run with the replacement.

    Wrap Ansi Now Has Code Coverage

I added code-coverage support, to help ensure that I wasn't making a mess while undertaking this refactor:

screen shot 2015-10-04 at 2 23 24 pm

To enable the coverage badge:

  1. visit https://coveralls.io/ and enable code-coverage for this repo.
  2. set the COVERALLS_TOKEN repo in travis.

Let me know anything you'd like changed in this pull, would love to use this as a drop in replacement in yargs.

reviewers: @dthre, @sindresorhus, @phated, @nexdrew

dthree commented 9 years ago

:+1:

One question: you're defaulting to soft, which I understand is modeled off of node-wordwrap. However, I believe wrap-ansi previously defaulted to hard, so this looks like it would break the existing expected functionality.

Not a big deal as it's a very new module, and my project is the only dependency :smiley: . I don't mind patching the break in my module, however, I don't really know that soft should be the default, just because substack did that. IMHO a hard wrap seems like it would be the more common choice.

bcoe commented 9 years ago

@dthree I'm pretty sure keeping the behavior soft maintains close to the original logic. In the algorithms's prior form, this was the logic for splitting the string:

if (++visible >= cols - 2 && lastSpace > 0) {

A newline character would be spliced into the string, if a `had been observed since the lastnewline` was injected. Try this with the current master branch:

require('./')('hello-world-how-are-you-today', 5)

Because there are no spaces to split on, you'll get a single line printed that looks like this:

hello-world-how-are-you-today.

If you opt to use the new hard option, you'll get:

hello
-worl
d-how
-are-
you-t
oday

The latterr ^ is better for building multi-column UIs such as yargs.

dthree commented 9 years ago

Okay I get it, that makes sense.

On the readme, the use of option isn't explicit, i.e. the viewer has no idea one has to use hard as the key in {hard: true}. Could you add a clarification there?

bcoe commented 9 years ago

@dthree sure thing, I'm tempted to add a few more unit tests for specific edge-cases too, before I move yargs over -- handling ANSI colors will be so slick!

dthree commented 9 years ago

:) Go for it. More unit tests can't hurt.

As this is a lot of changes, I think @sindresorhus should have the final word on this PR.

Qix- commented 9 years ago

Hey - can you separate out the code coverage into its own pull request?

bcoe commented 9 years ago

closing in favor or https://github.com/chalk/wrap-ansi/pull/4, https://github.com/chalk/wrap-ansi/pull/5 CC: @Qix

nexdrew commented 9 years ago

@bcoe Looks like this PR wasn't actually closed.