Skip to content
This repository has been archived by the owner on Sep 21, 2021. It is now read-only.

Fix bugs in cleanText() and wordCount(), add some tests #13

Merged
merged 6 commits into from Sep 20, 2016

Conversation

antislice
Copy link
Contributor

I made some editorial decisions, mostly around how hyphenated words and email addresses should be handled. The behavior should be all documented in the mocha tests. This also resolves the issues brought up in PRs #9 and #11.

Copy link
Owner

@cgiffard cgiffard left a comment

Choose a reason for hiding this comment

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

Really nice! Thanks so much for helping out — the library definitely needed it after I neglected it so much.

Test suite looks great — your contribution is much appreciated. I'll publish a new version to npm when I get to work. :)

@@ -0,0 +1 @@
--reporter nyan
Copy link
Owner

Choose a reason for hiding this comment

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

Heh.

assert.equal(ts.text, 'Hello hi friend.');
});

it('should replace em-dash with spaces'); // can I do that?
Copy link
Owner

Choose a reason for hiding this comment

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

Em dash is usually used as a word terminator in my experience, rather than to create hyphenated pairs, so I don't see why not!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, my comment there is more about figuring out where to get the emdash. I was apparently in too much of a hurry to copy/paste it.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, cool. Sorry, didn't intend to come across as patronising! Apologies if that was the case.

Not sure what OS you use, but I love em-dash, and there's a nice shortcut for it on Mac OS — option+shift+dash. Option+dash will give you an en-dash. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, I didn't read it as patronizing. My comment wasn't super clear.

@cgiffard cgiffard merged commit 2d1f0d6 into cgiffard:master Sep 20, 2016
This was referenced Sep 20, 2016
@antislice
Copy link
Contributor Author

Thanks for merging!

@cgiffard
Copy link
Owner

@antislice No, thank you. :)

@antislice antislice deleted the upstream-pr branch September 22, 2016 22:50
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.

None yet

2 participants