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

RFC: infallible JsString::new() constructor #21

Merged
merged 1 commit into from
Jul 25, 2018

Conversation

dherman
Copy link
Contributor

@dherman dherman commented Jun 18, 2018

This draft RFC proposes a backwards-incompatible change to the JsString::new() API to make it infallible, along with a JsString::try_new() companion API for explicitly handling the (rare) error case of overflowing the maximum string length limit.

Rendered

…ing::new()` API to make it infallible, along with a `JsString::try_new()` companion API for explicitly handling the (rare) error case of overflowing the maximum string length limit.
Copy link

@tptee tptee left a comment

Choose a reason for hiding this comment

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

I think the convenience factor here far outweighs the tiny edge case that can panic!. The explicit StringOverflow error helps a lot too–I remember interacting with JsString in the past and having no idea what could cause a None or how to handle it.

@dherman
Copy link
Contributor Author

dherman commented Jul 5, 2018

📢 This RFC is going into final comment period! We will plan to merge it within a week if there aren't significant new issues that arise in the next week. 📢

@dherman dherman added the final comment period last opportunity to discuss the proposed conclusion label Jul 5, 2018
@dherman dherman merged commit 1f137fc into neon-bindings:master Jul 25, 2018
@dherman dherman deleted the string-constructor branch July 25, 2018 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final comment period last opportunity to discuss the proposed conclusion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants