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

Fix for 0 length new String #404

Merged
merged 4 commits into from
May 19, 2020
Merged

Fix for 0 length new String #404

merged 4 commits into from
May 19, 2020

Conversation

tylermorten
Copy link

This Pull Request fixes/closes #398 .

It changes the following:

  • Adds the field length with the proper length of the string to the context
  • Adds a test for testing the fields is set correctly.

Copy link
Member

@Razican Razican left a 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?

@Razican Razican added the bug Something isn't working label May 18, 2020
@tylermorten
Copy link
Author

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?

@Razican
Copy link
Member

Razican commented May 18, 2020

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 :)

Copy link
Member

@Razican Razican left a 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?

Copy link
Member

@HalidOdat HalidOdat left a 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. :)

@Razican Razican added this to the v0.8.0 milestone May 19, 2020
@Razican Razican merged commit d84d9cb into boa-dev:master May 19, 2020
@jasonwilliams
Copy link
Member

Sorry for slow response but yes it looks good!

Thanks @TylerMorton

@tylermorten
Copy link
Author

Thank you! I'll continue to look through issues to see if there is anything else I can help on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

String() does not set length on instance
4 participants