Skip to content

add slice #104

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

Merged
merged 9 commits into from
Mar 30, 2018
Merged

add slice #104

merged 9 commits into from
Mar 30, 2018

Conversation

themattchan
Copy link
Contributor

No description provided.

-- | Returns the substring at indices [begin, end).
-- | If either index is negative, it is normalized to `length s - index`,
-- | where `s` is the input string. An empty string is returned if either
-- | index is out of bounds or if `begin > end`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to say "or if begin > end after normalisation".

@michaelficarra
Copy link
Contributor

I don't like the empty string being used as the error case. What else can we do?

@themattchan
Copy link
Contributor Author

Agreed. That is the default behaviour for javascript, but I could wrap the result in a Maybe.

@michaelficarra
Copy link
Contributor

I don't know if this is better. This implementation doesn't check for the bounds, it just returns Nothing on empty string results. That's not good. slice 0 0 should produce a "positive" result on any string.

@davidchambers
Copy link
Contributor

Feel free to copy code from the implementation of S.slice, if helpful. :)

@davidchambers
Copy link
Contributor

This is (necessarily) confusing code. It deserves some tests. ;)

@michaelficarra
Copy link
Contributor

The length calculations and bounds checking can be done in pure code. We should try to minimise the amount of JS. Basically, it should just expose Array.prototype.slice directly.

Copy link
Contributor

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

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

LGTM. Let's also add exhaustive tests for each of the cases that could produce Nothing.

@michaelficarra
Copy link
Contributor

Ping @themattchan. With only a few added tests, we can land this PR.

@themattchan
Copy link
Contributor Author

Ok, I think the new tests exercise all the error conditions.

Copy link
Contributor

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

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

💯 Thanks @themattchan! LGTM.

@garyb
Copy link
Member

garyb commented Mar 30, 2018

Thanks all!

@garyb garyb merged commit 6fce657 into purescript:master Mar 30, 2018
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.

4 participants