chalk / wrap-ansi

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

adds hard wrapping functionality #5

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

  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.

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

note: I started out by trying to tack this functionality onto the existing algorithm, but it turned out to be a hard enough problem that refactoring the algorithm seemed like the better approach.

nexdrew commented 9 years ago

@bcoe Need to rebase and drop commit 4181246 from this branch, I think. :ok_hand:

bcoe commented 9 years ago

@nexdrew I purposefully forked off of the code-coverage branch, my plan was to add a few more tests to this refactor to get coverage up a bit.

nexdrew commented 9 years ago

Ok, sorry. Ignore me.

sindresorhus commented 9 years ago

@dthree Can you review too?

dthree commented 9 years ago

@sindresorhus yes :)

dthree commented 9 years ago

This is looking :+1: to me at this point.

bcoe commented 9 years ago

@dthree @sindresorhus if you felt like giving me commit access, I'd be happy to land a few more tests on this branch before merging -- I'd like to get coverage a bit closer to 100%, and test all the weird edge-cases of wrapping.

dthree commented 9 years ago

@sindresorhus I'm going to merge this to start.

Wouldn't mind giving him access to this repo if you're fine with that, he's put a ton of work into it.

sindresorhus commented 9 years ago

Done! Was planning to do so after this PR anyways. Really appriciate the work you've done on this @bcoe :)

sindresorhus commented 9 years ago

Looks good to me too.

sindresorhus commented 9 years ago

Landed! :)

732852c4-67d8-11e3-90c3-576016e1aac4

dthree commented 9 years ago

Haha :) Thanks for all your work @bcoe