-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
Fix for 0 length new String #404
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! thank you for the pull request! It looks good, but it seems that this has formatting issues. Could you run cargo fmt
? Also, could you add a test for a string with UTF-8 (non-ASCII) characters?
Thanks! For the UTF-8 encoding, I'm guessing you want to support the count of the characters - not the byte length, correct? It appears that when I check the length of a String in node it gives me back the character count, not the byte count. I believe that the default for std::String len() returns the byte count...perhaps chars().count() would work better for this implementation? |
I think so, yes, I saw you already implemented this, let me give it another look :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for those changes! I think this is ready to be merged. We could include it in 0.8, since we have not released it yet. What do you think, @jasonwilliams, @HalidOdat?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks perfect to me!
Yes. We can include this in version 0.8. :)
Sorry for slow response but yes it looks good! Thanks @TylerMorton |
Thank you! I'll continue to look through issues to see if there is anything else I can help on. |
This Pull Request fixes/closes #398 .
It changes the following: