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

Add functions to get JsString as UTF-16 #944

Merged
merged 7 commits into from
Nov 29, 2022

Conversation

dnaka91
Copy link
Contributor

@dnaka91 dnaka91 commented Nov 15, 2022

Extending the API of JsString to retrieve the value as UTF-16 encoded Vec<u16>. Also, added a size_utf16 function for completeness.

I added a few code samples to the API docs for JsString to show at least some basic uses, as the docs felt a bit empty for this rather core component.

Fixes #943

@kjvalencik
Copy link
Member

@dherman can you take a look at the docs in this since you are actively working on them in #942?

crates/neon/src/sys/bindings/functions.rs Show resolved Hide resolved
crates/neon/src/sys/string.rs Outdated Show resolved Hide resolved
crates/neon/src/types_impl/mod.rs Show resolved Hide resolved
crates/neon/src/types_impl/mod.rs Outdated Show resolved Hide resolved
crates/neon/src/types_impl/mod.rs Show resolved Hide resolved
test/napi/lib/strings.js Show resolved Hide resolved
@dherman
Copy link
Collaborator

dherman commented Nov 19, 2022

@dherman can you take a look at the docs in this since you are actively working on them in #942?

These look nice! I'll just stay out of the JsString docs in that PR for now, and then afterwards I can do a followup PR to touch up any stylistic inconsistencies.

@dherman dherman mentioned this pull request Nov 19, 2022
15 tasks
@dnaka91
Copy link
Contributor Author

dnaka91 commented Nov 21, 2022

Sorry for the delay. Updated the PR with the requested changes, and I resolved the discussions accordingly. Feel free to re-open them if I missed anything.

@dnaka91
Copy link
Contributor Author

dnaka91 commented Nov 29, 2022

Is there anything to be done from my side, or is this just missing a final review?

Would be great to have these extra APIs available soon-ish, but I might just point cargo to my PR branch in the meantime then 👍

@kjvalencik
Copy link
Member

Should be just the final review. I should have a chance this week. I haven't had a chance with Thanksgiving last week.

Copy link
Member

@kjvalencik kjvalencik left a comment

Choose a reason for hiding this comment

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

This looks really great! Thanks for this addition and the fantastic docs, examples and tests!

@kjvalencik kjvalencik merged commit 681a71d into neon-bindings:main Nov 29, 2022
@dnaka91 dnaka91 deleted the string-utf16 branch November 29, 2022 17:06
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.

Support for UTF-16 strings
3 participants