Skip to content

lastIndexOf' with index greater than string length returns Nothing #91

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

Closed
hdgarrood opened this issue Dec 16, 2017 · 4 comments · Fixed by #137
Closed

lastIndexOf' with index greater than string length returns Nothing #91

hdgarrood opened this issue Dec 16, 2017 · 4 comments · Fixed by #137

Comments

@hdgarrood
Copy link
Contributor

hdgarrood commented Dec 16, 2017

In the JavaScript API, the fromIndex parameter in str.lastIndexOf(pattern [, fromIndex]), perhaps surprisingly, represents the index at which to stop searching (if we imagine the search proceeds from left-to-right). This means that if you provide a fromIndex which is greater than or equal to the string's length, in JS, it's equivalent to searching the whole string, i.e. not specifying that parameter at all. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/lastIndexOf

However, in this library, the bounds check in lastIndexOf' means that e.g. lastIndexOf' (Pattern "a") 3 "aa" comes out as Nothing. I'd suggest getting rid of this check entirely, so that this matches better with the JS behaviour. This is a breaking change unfortunately.

Incidentally there are one or two other breaking changes I'd like to do (#81, #78, and also renaming Data.String.CodePoints to Data.String and renaming Data.String to Data.String.CodeUnits) so maybe we could do all of these the next time we break everything (i.e. when we release 0.12?)

@hdgarrood
Copy link
Contributor Author

Previous discussion: #55.

@hdgarrood
Copy link
Contributor Author

Can we do this as part of the updates for 0.14?

@garyb
Copy link
Member

garyb commented Feb 23, 2020

This is one of those sneaky hidden breaking changes that isn't communicated in the types, it's a shame there's not a good way of knowing how much code this is likely to affect 😕

@hdgarrood
Copy link
Contributor Author

I think this is such a weird behaviour that someone almost certainly would have opened an issue by now if they had actually run into it.

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 a pull request may close this issue.

2 participants