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 startsWith and endsWith #127

Open
triallax opened this issue Sep 24, 2020 · 12 comments · May be fixed by #147
Open

Add startsWith and endsWith #127

triallax opened this issue Sep 24, 2020 · 12 comments · May be fixed by #147
Labels
type: enhancement A new feature or addition.

Comments

@triallax
Copy link
Contributor

These functions are already present in purescript-stringutils but many people, especially ones coming from JavaScript, expect them to be present in the main strings library.

Should I make a PR to fix this?

@garyb
Copy link
Member

garyb commented Sep 24, 2020

I would say I'm not really in favour of it, personally. We're not trying to replicate the JS API here, and it already has stripPrefix and stripSuffix which can be used for this purpose - generally these functions are more useful too, as often a startsWith would be followed by chopping the prefix off too. Not having the test-only versions hopefully means people will stumble over these more useful functions, rather than being stuck in their JS ways.

That's just my opinion though 🙂

@natefaubion
Copy link
Contributor

To be clear, startsWith prefix is isJust <<< stripPrefix prefix.

@triallax
Copy link
Contributor Author

triallax commented Sep 24, 2020

@garyb you do make a good point. I just think that the discoverability of this is low for new people. Should I add an example to the cookbook?

@garyb
Copy link
Member

garyb commented Sep 24, 2020

Sure! That sounds like a good idea.

@JordanMartinez
Copy link
Contributor

I actually ran into this issue a week ago. I ended up using stringutils because my goal was to solve my problem (does this string start with "X"?), not think about how I could achieve my goal via some other way. For some reason, stripPrefix never stood out to me as being helpful.

So, I would agree that discoverability is the issue here.

@natefaubion
Copy link
Contributor

FWIW I've somewhat often wanted startsWith/endsWith. I would not be opposed to adding them with docs that refer to stripPrefix/stripSuffix.

@triallax
Copy link
Contributor Author

I'm not sure what to do here. Should I open a PR here and add startsWith and endsWith like @natefaubion is suggesting, or should I add examples to the cookbook?

@natefaubion
Copy link
Contributor

@mhmdanas I'm just throwing in a point-of-view that startsWith/endsWith are not always functions you don't want. I've written lots of code that checks a prefix to classify something, but still needs to retain the prefix for convenience. I'm not really fighting for their inclusion, other than saying that it's expected functionality, and I think it's fine if we did include them while also suggesting that stripPrefix/stripSuffix may be more appropriate.

@wclr
Copy link

wclr commented Mar 10, 2021

What about performance, is it not a consideration? I mean if this is a standard functions of String in JS.

@triallax triallax linked a pull request Mar 10, 2021 that will close this issue
7 tasks
@hdgarrood
Copy link
Contributor

Stripping a string prefix is cheap (i.e. O(1)) in JS because JS strings are immutable. We should avoid unnecessary FFI here because that creates problems for alternative backends.

@triallax
Copy link
Contributor Author

Stripping a string prefix is cheap (i.e. O(1)) in JS because JS strings are immutable.

We can't be sure that is the case on other backends (it might as well be for at least most; I'm not sure).

We should avoid unnecessary FFI here because that creates problems for alternative backends.

I don't think it should be much of a problem because it's just two extra functions, but yeah, it's still two more functions to implement in the FFI.

@hdgarrood
Copy link
Contributor

hdgarrood commented Mar 10, 2021

I think a backend which uses a mutable type for Strings is probably incorrect; if you want to write a PureScript backend for a language which does not provide an immutable String type, then you should probably map PureScript strings to some other type in your backend which is an immutable string. We should really write a spec for what is expected of a backend, and specifically how the primitive types work. There are also problems around the encoding of a String as well (whether invalid UTF-16 is allowed, for instance).

@JordanMartinez JordanMartinez added the type: enhancement A new feature or addition. label Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A new feature or addition.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants