Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

+added 'partition', and 'repartition' #5

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

lsauer
Copy link

@lsauer lsauer commented Apr 30, 2013

a great lib.

@lsauer
Copy link
Author

lsauer commented May 2, 2013

I don't know why github considers some files to be changed entirely (Encoding or EOL sign change?) Can you still work with this?

@lsauer lsauer closed this May 2, 2013
@lsauer lsauer reopened this May 2, 2013

// Partitions a string and subsequently joins all parts
slang.repartition = function repartition(input, nblock, concat) {
return input.split(RegExp("(.{"+nblock+"})","gm")).filter(Boolean).join(concat);
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think we can rely on filter being available cross browser. Maybe try input.match(RegExp(".{1," + n + "}", "gm") instead to avoid empty strings all together?

Copy link
Author

Choose a reason for hiding this comment

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

I tried hard yet could not find a way to make split avoid empty results. m.buettner comes to the same conclusion here. I will just filter using a for-loop instead...

@devongovett
Copy link
Owner

Thanks for the pull request! I've added several comments inline. One more: is there any reason why the documentation HTML page generated by docco was moved to slang_alternative_docs.html instead of just index.html as before? I get that lots of these files are from the new version of docco and I can easily rename that file, but just wondering.

Thanks again! I'll merge after these few small issues are fixed.

@lsauer
Copy link
Author

lsauer commented May 5, 2013

The slang_alternative_docs.html should be named index_mobile.html.The docco.css was accidentally copied over, hence index.html looks like a mess. To be fixed. Update: docco.css is responsive anyways, so I removed any other html-files.

@lsauer
Copy link
Author

lsauer commented Nov 4, 2013

Hi there. Please review...



// Test **slang.repartition**
assert.equal('Hel@lo @wor@ld!', slang.repartition('Hello world!'));
Copy link
Owner

Choose a reason for hiding this comment

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

Where do the @'s come from in this? Is this missing some arguments?

@devongovett
Copy link
Owner

Added a few more small things inline. Is it possible for you to revert the docs changes and minified JS changes on this PR so there aren't so many files? I will regenerate the docs and minified version after merging. So if possible, just include the code changes, test changes, and README changes. If it's easier, you could open a new PR. Thanks!

@devongovett
Copy link
Owner

Were you going to finish this or shall I close it? Right now the tests don't pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants