Skip to content

Make splitAt total #87

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 7 commits into from
May 18, 2018
Merged

Conversation

MonoidMusician
Copy link
Contributor

Fixes #78, breaking change.

Copy link
Contributor

@hdgarrood hdgarrood left a comment

Choose a reason for hiding this comment

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

Thanks! I'm in favour of course but I'll wait and see what other maintainers think.

@hdgarrood
Copy link
Contributor

Oh actually, do you think you could update the comments so that it's clear that with r = splitAt i str, the string r.before will have exactly i characters (unless the original string didn't contain that many)?

@paf31
Copy link
Contributor

paf31 commented Aug 11, 2017

👍 from me, but I assume we're going to wait for the next round of breaking changes?

Copy link
Contributor

@hdgarrood hdgarrood left a comment

Choose a reason for hiding this comment

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

Oh actually I've just realised, Data.String.CodePoints needs a corresponding change at the same time.

Speaking of the next round of breaking changes, were we going to do that before or after 0.12?

Regarding the comments, I think with what you have there, it would be reasonable to expect splitAt 0 "hi" to return { before: "h", after: "i" }, so I think it would be good to make it absolutely clear (while this is at the front of my mind). How about this:

Returns a string split into two substrings at the given index, where before includes all of the characters up to (but not including) the given index, and after is the rest of the string, from the given index on. That is, (splitAt i str).before contains exactly i characters (code units), unless i is negative (in which case it will be the empty string), or str itself is not that long (in which case it is equal to str).

@MonoidMusician
Copy link
Contributor Author

Thanks for the review! Does this look good?

splitAt :: Int -> String -> Maybe { before :: String, after :: String }
-- | Splits a string into two substrings, where `before` contains the code
-- | points up to (but not including) the given index, and `after` contains the
-- | rest of the string, from that index on.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not have the same comment here as in Data.String?

@hdgarrood
Copy link
Contributor

Other than my last comment I think this looks great now, thank you!

@MonoidMusician
Copy link
Contributor Author

Okay, thanks!

@garyb garyb changed the base branch from master to compiler/0.12 May 18, 2018 14:39
@garyb garyb merged commit 62c9fdc into purescript:compiler/0.12 May 18, 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