Add wrap function #410

bsteephenson · 2015-03-04T20:26:13Z

Adds a line wrapping function that limits line length to a certain width
In response to issue #278

Let me know if there's anything more I should do (more tests, documentation, etc). I'm glad to help out.

stoeffel · 2015-03-04T20:35:11Z

I will have a look at it on the WE.Can you add the documentation to the README.markdown? Thanks for the PR.

stoeffel · 2015-03-08T08:37:14Z

test/strings.js

+    equal(_.wrap("My name is", 2, '.', false), "My .name .is", 'works with width 2 and cut = false');
+    equal(_.wrap("My name is", 2, '.', true), "My. n.am.e .is", 'works with width 2 and cut = true');
+    equal(_.wrap("My name is", 3, '.', false), "My .name .is", 'works with width 3 and cut = true');
+    equal(_.wrap("My name is", 3), "My \nname \nis", 'Default parameters work');


You should add more tests.

edgecases

defaults

Checkout the other tests.

stoeffel · 2015-05-02T09:53:11Z

Can you rebase this? Thanks

bsteephenson · 2015-05-02T11:23:39Z

Rebased with a mocha test
Let me know if there's anything else I should do =)

stoeffel · 2015-05-02T12:39:53Z

If you add this test:
equal(wrap("My name is", -1), "My name is", "Just return original line if width <= 0")
You have 100% coverage.

Otherwise it looks good to me.

bsteephenson · 2015-05-02T17:07:51Z

Done and squashed.

stoeffel · 2015-05-03T11:19:58Z

wrap.js

+		return str;
+	}
+	else if(!cut){
+		words = str.split(" ");


How about we use `underscore.string/words' here?

bsteephenson · 2015-05-04T01:33:37Z

Let's talk about the specifics of what this is supposed to do.

wrap('this is a sentence', 4, \n, false)

What should this return? Right now it would return 'this \nis a \nsentence' which I'm realizing now doesn't make much sense. Would 'this\nis a\nsentence' be more obvious?

stoeffel · 2015-05-04T06:22:38Z

Would 'this\nis a\nsentence' be more obvious?

Yes, I think so.

PHP's wordwrap also drops spaces at the end. http://us3.php.net/manual/en/function.wordwrap.php
We could add an option for this, but it already has 4 arguments...

esamattis · 2015-05-04T13:43:16Z

Options object maybe. Could be done as backwards compatible with type checking.

stoeffel · 2015-05-06T06:46:04Z

👍 for an options object I could think of some other options we could add in the future f.e. dontBreakHTMLTags

bsteephenson · 2015-05-13T19:08:04Z

Now wrap('this is a sentence', {width: 4, seperator: '\n', cut: false}) returns 'this\nis a\nsentence'

stoeffel · 2015-05-13T21:08:51Z

Options object looks good. Could you add an option for dropping trailing spaces as discussed above? And updat the readme? Thanks for your work

bsteephenson · 2015-05-13T23:27:30Z

I added a trailingSpaces options which, if true, fills each line with trailing spaces to make them at least width long (I'm not sure if this is what you meant). I also updated the Readme.

stoeffel · 2015-05-15T07:03:50Z

Sorry, I actually meant this:

What should this return? Right now it would return 'this \nis a \nsentence' which I'm realizing now doesn't make much sense. Would 'this\nis a\nsentence' be more obvious?

Maybe call the option preserveSpace. Sry for the confusion.

stoeffel · 2015-05-15T07:16:47Z

but the trailingSpaces option is nice too 👍

stoeffel · 2015-05-31T11:31:30Z

@bsteephenson Would be great if you would add the preserveSpaces (or take a better name) option so we can merge this.

Thanks

bsteephenson · 2015-06-26T19:41:57Z

Working on this atm. I am having preserveSpaces have a higher precedence over trailingSpaces.

stoeffel · 2015-06-27T09:20:43Z

README.markdown

@@ -269,6 +269,25 @@ lines("Hello\nWorld");
 // => ["Hello", "World"]
 ```

+#### wrap(str, options) => string
+
+Splits a line `str` (default '') into several lines of size `options.width` (default 75) using a `options.seperator` (default '\n'). If `options.trailingSpaces` is true, make each line at least `width` long using trailing spaces. If `options.cut` is true, create new lines in the middle of words. If `options.trailingSpaces` is true, preserve the space that should be there at the end of a line.


options.trailingSpaces is true, preserve the space that should be there at the end of a line.

This should be options.preserveSpaces

stoeffel · 2015-06-27T09:27:09Z

Besides my comments in the readme, this looks good to me. Thx a lot

bsteephenson · 2015-06-28T02:13:39Z

Corrected all the oops's in Readme
Let me know if you want me to squash all the commits into one

stoeffel · 2015-06-28T06:30:51Z

Squashing would be nice. I want have time today to check it one more time. But I will review it again in the beginning of next week and then merge it.
Thanks for your work.

stoeffel · 2015-06-30T15:12:33Z

Did check it again. Could you squash? I will merge it.

Adds a line wrapping function that limits line length to a certain width Added documentation and tests

bsteephenson · 2015-06-30T19:59:33Z

Done =)

Add wrap function

stoeffel · 2015-07-01T09:31:56Z

nice thanks a lot.

cahrens · 2016-02-19T02:53:02Z

@stoeffel when "wrap" was added, it wasn't added to the "exports" function as a conflict with underscore. I know mixing in underscore.string is now discouraged, but unfortunately we have a lot of old code written with the mixin paradigm. I am having difficulty upgrading underscore.string from 2.x due to the wrap conflict.

stoeffel · 2016-02-19T06:37:32Z

Hey @cahrens. Good catch. Would be awesome if you could open a PR to include wrap in the exports.
I want to merge #481 and #482 first, before I publish the next version (if you have time and willing to review these two PRs would be cool).

Thanks

cahrens · 2016-02-19T13:31:03Z

Sure, I will looking into it this weekend. I would like to add a test that checks for conflicting functions between underscore and underscore.string.exports. I haven't looked at your test framework yet, but hope it's possible to add a test-only dependency on underscore.

cahrens · 2016-02-19T20:55:55Z

@stoeffel I see that underscore is already a development dependency. Can you point me to documentation on how to run the tests locally? I poked around but couldn't find anything.

Fixes a bug introduced in esamattis#410.

stoeffel reviewed Mar 8, 2015
View reviewed changes

stoeffel added the candidate label May 2, 2015

bsteephenson force-pushed the master branch from 3609f51 to 827aacf Compare May 2, 2015 11:21

bsteephenson force-pushed the master branch from 827aacf to e7a5804 Compare May 2, 2015 17:07

stoeffel reviewed May 3, 2015
View reviewed changes

wrap.js

return str;

}

else if(!cut){

words = str.split(" ");

Copy link

Collaborator

stoeffel May 3, 2015

Choose a reason for hiding this comment

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

How about we use `underscore.string/words' here?

stoeffel reviewed Jun 27, 2015
View reviewed changes

Add wrap function

4c8a7b0

Adds a line wrapping function that limits line length to a certain width Added documentation and tests

bsteephenson force-pushed the master branch from 31ae93f to 4c8a7b0 Compare June 30, 2015 16:49

stoeffel added a commit that referenced this pull request Jul 1, 2015

Merge pull request #410 from BSteephenson/master

6b3a278

Add wrap function

stoeffel merged commit 6b3a278 into esamattis:master Jul 1, 2015

cahrens pushed a commit to cahrens/underscore.string that referenced this pull request Feb 20, 2016

Add "wrap" to exports function.

f77aa36

Fixes a bug introduced in esamattis#410.

cahrens pushed a commit to cahrens/underscore.string that referenced this pull request Feb 20, 2016

Add "wrap" to exports.

4d08855

Fixes a bug introduced in esamattis#410.

cahrens pushed a commit to cahrens/underscore.string that referenced this pull request Feb 20, 2016

Add "wrap" to exports.

c1a3b89

Fixes a bug introduced in esamattis#410.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add wrap function #410

Add wrap function #410

bsteephenson commented Mar 4, 2015

stoeffel commented Mar 4, 2015

stoeffel Mar 8, 2015

stoeffel commented May 2, 2015

bsteephenson commented May 2, 2015

stoeffel commented May 2, 2015

bsteephenson commented May 2, 2015

stoeffel May 3, 2015

bsteephenson commented May 4, 2015

stoeffel commented May 4, 2015

esamattis commented May 4, 2015

stoeffel commented May 6, 2015

bsteephenson commented May 13, 2015

stoeffel commented May 13, 2015

bsteephenson commented May 13, 2015

stoeffel commented May 15, 2015

stoeffel commented May 15, 2015

stoeffel commented May 31, 2015

bsteephenson commented Jun 26, 2015

stoeffel Jun 27, 2015

stoeffel commented Jun 27, 2015

bsteephenson commented Jun 28, 2015

stoeffel commented Jun 28, 2015

stoeffel commented Jun 30, 2015

bsteephenson commented Jun 30, 2015

stoeffel commented Jul 1, 2015

cahrens commented Feb 19, 2016

stoeffel commented Feb 19, 2016

cahrens commented Feb 19, 2016

cahrens commented Feb 19, 2016

Add wrap function #410

Add wrap function #410

Conversation

bsteephenson commented Mar 4, 2015

stoeffel commented Mar 4, 2015

stoeffel Mar 8, 2015

Choose a reason for hiding this comment

stoeffel commented May 2, 2015

bsteephenson commented May 2, 2015

stoeffel commented May 2, 2015

bsteephenson commented May 2, 2015

stoeffel May 3, 2015

Choose a reason for hiding this comment

bsteephenson commented May 4, 2015

stoeffel commented May 4, 2015

esamattis commented May 4, 2015

stoeffel commented May 6, 2015

bsteephenson commented May 13, 2015

stoeffel commented May 13, 2015

bsteephenson commented May 13, 2015

stoeffel commented May 15, 2015

stoeffel commented May 15, 2015

stoeffel commented May 31, 2015

bsteephenson commented Jun 26, 2015

stoeffel Jun 27, 2015

Choose a reason for hiding this comment

stoeffel commented Jun 27, 2015

bsteephenson commented Jun 28, 2015

stoeffel commented Jun 28, 2015

stoeffel commented Jun 30, 2015

bsteephenson commented Jun 30, 2015

stoeffel commented Jul 1, 2015

cahrens commented Feb 19, 2016

stoeffel commented Feb 19, 2016

cahrens commented Feb 19, 2016

cahrens commented Feb 19, 2016