-
Notifications
You must be signed in to change notification settings - Fork 469
Add missing docstring for String #7571
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
base: master
Are you sure you want to change the base?
Conversation
runtime/Stdlib_String.res
Outdated
@@ -1,6 +1,6 @@ | |||
type t = string | |||
|
|||
@val external make: 'a => string = "String" | |||
@new external make: 'a => string = "String" |
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.
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.
Yeah, that sounds right, but how does setSymbol
work then?
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.
Sadly, it won't. Seems will only work on string object instances.
So the binding is incorrect IMHO. We'd need another abstract type for string object instances, and could then define getSymbol
/setSymbol
on that.
I assume this is a pretty niche use case anyway. Personally I never tried setting a value with a symbol key on a string before. Maybe we should just remove the getSymbol
/setSymbol
bindings from the stdlib?
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.
Maybe we should just remove the getSymbol/setSymbol bindings from the stdlib?
Yes, I think that makes more sense.
This one is a bit weird, I had to make the
@val
to@new
change to make the examples work.I noticed this in plain JavaScript as well, that not having the
new
keyword, doesn't make it work.Happy to hear your thoughts here, not sure what is best.