Skip to content

Conversation

bloodyowl
Copy link
Collaborator

@bobzhang rebased against master so it might be easier to PR to master instead of #2758.

bobzhang and others added 2 commits April 2, 2019 11:29
Add some functions to Belt.String
@bloodyowl bloodyowl force-pushed the belt_String_update branch from 687f92d to e74cd69 Compare April 2, 2019 10:16
@bobzhang
Copy link
Member

bobzhang commented Apr 3, 2019

Before merging it, it would be helpful if we have a second eye to have a look if everything is okay.
I will leave it here until April 10

@bloodyowl
Copy link
Collaborator Author

@bsansouci @phated : I'm pinging as you were active lately on the original PR, how does that look to you?

Benjamin: how would you feel about this on the native side?

Copy link
Contributor

@MoOx MoOx left a comment

Choose a reason for hiding this comment

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

Looks good to me (beside the substr thing)

substr "abcdefghij" ~from: 12 ~len: 2= ""
]}
*)
external substr : t -> from:int -> len:int -> t = "substr" [@@bs.send]
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO substr should be replaced by substring as it's not recommended to use it, see below

Warning: Although String.prototype.substr(…) is not strictly deprecated (as in "removed from the Web standards"), it is considered a legacy function and should be avoided when possible. It is not part of the core JavaScript language and may be removed in the future. If at all possible, use the substring() method instead.

(source https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/substr)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

given String.prototype.substring is equivalent to String.prototype.slice (index + endIndex) and String.prototype.substr is index + length, I figured it would be interesting to leave the two ways of doing so.

if the substr signature isn't really interesting for users (I'm not sure if widely used), we can remove it altogether.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah considering the fact that substring≃slice, feel free to forget what I said :)

Nice read about diff between substring & slice https://stackoverflow.com/questions/2243824/what-is-the-difference-between-string-slice-and-string-substring#2243835

substrToEnd "abcdefghij" ~from: 12 = ""
]}
*)
external substrToEnd : t -> from:int -> t = "substr" [@@bs.send]
Copy link
Contributor

Choose a reason for hiding this comment

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

If we decide to go for substring, I guess we should use substringToEnd here

@chenglou
Copy link
Member

chenglou commented Apr 5, 2019

Hold on; are we doing the substring type thing or not? Like Rust/Go/Swift
Feels like we should

@bloodyowl
Copy link
Collaborator Author

@chenglou what do you mean?

@chenglou
Copy link
Member

chenglou commented Apr 5, 2019

We were discussing about having a substring type. This is needed for native also, since native's string representation is much more naive. Substring is probably the best tradeoff we can make in terms of interop, perf, correctness (unicode) and UX: https://developer.apple.com/documentation/swift/substring

Though us talking about it has slowed down the progress to Belt.String api, which is unfortunate

@chenglou
Copy link
Member

chenglou commented Apr 6, 2019

(Discussed offline. @bloodyowl is looking into this

@adnelson
Copy link

adnelson commented May 7, 2020

what's the status of this? I'd love to have Belt.String!

@bloodyowl bloodyowl closed this Jan 31, 2021
@namenu namenu mentioned this pull request Aug 18, 2021
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.

5 participants