Skip to content
This repository was archived by the owner on Jul 4, 2025. It is now read-only.

Conversation

@pete-the-pete
Copy link
Contributor

Added some string processing tests

@rmurphey
Copy link
Owner

rmurphey commented Jun 7, 2013

Sorry for not getting to this sooner -- I'd like to pull this in, but I'd love it if you can update it to use camelCase for the variable names, per the style of the rest of the repo.

@pete-the-pete
Copy link
Contributor Author

Thanks for getting back to me, sorry it took me a while, i was out of town. Hopefully I fixed everything, but let me know if I missed something.

@rmurphey
Copy link
Owner

Thanks! One other thing, looks like there's not an app/strings.js file in this pull request? If you can add one and stub out the functions (like you see in other app/*.js files) that would be great and I'll get this in. Thanks for adding these!

@pete-the-pete
Copy link
Contributor Author

I added in the stubs, let me know if there's anything else you need me to do.

Copy link
Owner

Choose a reason for hiding this comment

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

The variable mid is going to end up just being the string's length, so it doesn't seem right to expect these two things to be equal.

@jamesplease
Copy link
Collaborator

@pete-the-pete, thanks so much for this PR! Sorry that it's taken a bit for it to get merged in, but I'm going to make these last few changes over in #102 before I merge it in. I'm going to close this PR in the meantime. Oh, and, don't worry – you'll still be the name on the commit that lands in master.

Thanks again :)

@jamesplease jamesplease closed this May 8, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants