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

Removed String converters #345

Closed
wants to merge 1 commit into from
Closed

Removed String converters #345

wants to merge 1 commit into from

Conversation

kkoopa
Copy link
Collaborator

@kkoopa kkoopa commented May 13, 2015

NanUtf8String and NanUcs2String just duplicate v8's equivalent classes. NanAsciiString is not really an ASCII string, true ASCII is a subset of UTF-8, so that can be used instead.

@kkoopa
Copy link
Collaborator Author

kkoopa commented May 16, 2015

Merged in efced26.

@kkoopa kkoopa closed this May 16, 2015
@kkoopa kkoopa deleted the nostring branch May 16, 2015 13:12
@springmeyer
Copy link
Contributor

@kkoopa - are you sure this is uneeded? I'm trying to port https://github.com/mapnik/node-mapnik/blob/a40d9ce8c6c44f201c5319d84c34d77f79072a9a/src/mapnik_palette.cpp#L45-L46 to Nan 2.0.5 and after switching to Nan::Utf8String I'm seeing test failures. It may be on my end, but I thought I'd let you know just in case you have ideas. I wonder if v8's string->WriteAscii behaves differently than string->WriteUtf8.

@kkoopa
Copy link
Collaborator Author

kkoopa commented Aug 17, 2015

They behave very differently. The old so called Ascii methods dealt in raw bytes with no actual encoding. This was rectified. The difference here is that you are not passing a valid ASCII string. Since ASCII is a subset of UTF-8, a valid ASCII string is valid UTF-8. Use a Buffer instead.

springmeyer pushed a commit to mapnik/node-mapnik that referenced this pull request Aug 17, 2015
@springmeyer
Copy link
Contributor

@kkoopa - Thank you! That fixed it :)

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.

2 participants