Skip to content

Conversation

narimiran
Copy link
Member

@narimiran narimiran commented Apr 6, 2018

Add check if sub in replace or sep in split is an empty string. If it is empty, return the original string.

Fixes #7507.

If it is empty, return the original string.
@dom96
Copy link
Contributor

dom96 commented Apr 6, 2018

Supersedes: #7509


doAssert "a largely spaced sentence".split(" ", maxsplit=1) == @["a", " largely spaced sentence"]
doAssert(sep.len > 0)
if sep.len == 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

perform this check in splitCommon, also make sure that rsplit has the same.

@dom96
Copy link
Contributor

dom96 commented Apr 6, 2018

Copying @GULPF's comment here:

Some comparisons with other languages, result after running "foo".replace("", "bar") or equivalent:
js, ruby: "barfoo"
python, java: "barfbarobarobar"
C#: error

Wouldn't it be a better idea to follow one of these examples instead of coming up with yet a different behaviour?

@dom96
Copy link
Contributor

dom96 commented Apr 6, 2018

Relevant IRC discussion: https://irclogs.nim-lang.org/06-04-2018.html#09:18:39

@dom96
Copy link
Contributor

dom96 commented Apr 7, 2018

We need to decide how to handle this. @Araq wants it to be a no op, I think adopting Python's semantics is a good idea (principle of least surprise and all that :)).

@Araq
Copy link
Member

Araq commented Apr 15, 2018

ok, so do it Python's way. Modify find that it returns the "first matching position" for the empty string.

@Araq
Copy link
Member

Araq commented Apr 18, 2018

FYI Python does not handle split and replace consistently either.

@Araq
Copy link
Member

Araq commented Apr 18, 2018

Fixed it differently.

@Araq Araq closed this Apr 18, 2018
@dom96
Copy link
Contributor

dom96 commented Apr 21, 2018

Relevant commit: 5d13e3f

@narimiran narimiran deleted the fix-7507 branch October 14, 2018 09:26
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.

3 participants