Skip to content

Conversation

bobzhang
Copy link
Member

@bobzhang bobzhang commented Apr 19, 2018

see #2759

@bobzhang bobzhang mentioned this pull request Apr 19, 2018
@cullophid
Copy link

Should we not choose lists over arrays when possible? e.g. String.split
switching between list and array seemingly at random (I know its not :)) will be very confusing for newcomers?

@andreypopp
Copy link

Could the API use named parameters? I always forget what is what when using values like this:

val replace : t -> t ->  t ->  t

@bsansouci
Copy link
Contributor

Hey could we get this merged? I'd love to have a split function finally lol

@bobzhang
Copy link
Member Author

bobzhang commented Oct 2, 2018

@andreypopp that makes sense.
What do other people think about this PR, it was halted since I am unclear the story on native side about string processing

@bsansouci
Copy link
Contributor

The native side will need some love. I can start implementing them as soon as you merge but I wouldn't mind some help :)

@phated
Copy link
Contributor

phated commented Dec 16, 2018

I've been writing many of these string helpers in pure Reason. Is there a desire to implement them as JS externals? If they were implemented in OCaml, wouldn't they be cross-platform?

@bsansouci
Copy link
Contributor

We can implement them as externals when compiling to JS and in Reason when compiling to native, that's probably the best of both worlds :)

@phated
Copy link
Contributor

phated commented Dec 16, 2018

@bsansouci I haven't reviewed everything but a potential downside would be if the JS APIs aren't the best APIs. For example, I'm writing a lot of my string methods to return Option.

@bsansouci
Copy link
Contributor

null in JS is modeled as option in native, so you're good. To be honest, these are just string manipulation functions, there's only so far you can go with their API. We might have another version of substr for native, just because there's the weirdness with substr vs substring in JS

@phated
Copy link
Contributor

phated commented Dec 17, 2018

@bsansouci I always felt like a goal of Belt was to remove the Js namespace from code? Doesn't nullable come from Js?

@bsansouci
Copy link
Contributor

Oh well I think we'll be able to / already can compile options to unboxed nulls in JS, so there's no downsides I see to using them in both JS and native. I meant more that on the "backend" side of things, we have the power to do things slightly differently (in this very specific case of optionals), so we don't always need to re-write in Reason.

@MoOx
Copy link
Contributor

MoOx commented Mar 20, 2019

I would love to see this released :)

]}
*)
external replace : t -> t -> t -> t = "" [@@bs.send]

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a good place to use label?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can PR and target your branch if you want.

I'm planning on also adding Belt.String.length, Belt.String.slice/sliceToEnd, Belt.String.get, Belt.String.indexOf if that's ok with you.

should it also include Belt.String.replaceRegex, Belt.String.matchRegex ou should they just remain in Js.String?

Copy link
Member Author

Choose a reason for hiding this comment

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

what is the type of indexOf, I suggest we avoid char type in the API, otherwise it looks good to me

Copy link
Collaborator

Choose a reason for hiding this comment

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

the type would be t -> t -> int option

@bloodyowl bloodyowl mentioned this pull request Apr 2, 2019
@cristianoc
Copy link
Collaborator

closing very old PRs

@cristianoc cristianoc closed this Jun 25, 2022
@cristianoc cristianoc deleted the belt_String branch July 18, 2022 05:48
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.

8 participants