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

Proposed solution for issue #8919 #9280

Merged
merged 2 commits into from
Oct 12, 2018

Conversation

eqperes
Copy link
Contributor

@eqperes eqperes commented Oct 10, 2018

Returns s.len + 1 when substring sub is empty, as proposed in the issue discussion.

@timotheecour
Copy link
Member

timotheecour commented Oct 11, 2018

why not throw an exception (eg: ValueError) instead when sub is empty? seems more reasonable to me rather than the seemingly arbitrary s.len+1 (yes I read the rationale regarding replace(s, "") which also seems like it should throw).
Throwing catches bugs.

@Araq
Copy link
Member

Araq commented Oct 11, 2018

This should instead use assert or doAssert, it's an API misuse.

@timotheecour
Copy link
Member

I agree with @Araq it should fail instead of silently accepting nonsensical inputs;
what about https://github.com/nim-lang/Nim/blob/devel/lib/pure/strutils.nim#L1520 case mentioned #8919 (which was rationale for the original fix that returned s.len+1)

I just tried,
replace("foo", "", "1") returns 1f1o1o1 ; shouldn't that also assert or doAssert according to same logic? would seem saner

@Araq
Copy link
Member

Araq commented Oct 11, 2018

I just tried,
replace("foo", "", "1") returns 1f1o1o1 ; shouldn't that also assert or doAssert according to same logic? would seem saner

This was discussed and we choose this implemementation because Python/JS/whatever also does it this way.

@eqperes
Copy link
Contributor Author

eqperes commented Oct 11, 2018

So, it should use doAssert sub.len > 0 and only accept non-empty substrings?

@Araq
Copy link
Member

Araq commented Oct 12, 2018

Exactly.

@eqperes
Copy link
Contributor Author

eqperes commented Oct 12, 2018

👍 done

@Araq Araq merged commit 14925ee into nim-lang:devel Oct 12, 2018
krux02 pushed a commit to krux02/Nim that referenced this pull request Oct 15, 2018
* Proposed solution for issue nim-lang#8919

* count sub/subs must be non-empty
narimiran pushed a commit to narimiran/Nim that referenced this pull request Oct 31, 2018
* Proposed solution for issue nim-lang#8919

* count sub/subs must be non-empty
narimiran pushed a commit to narimiran/Nim that referenced this pull request Nov 1, 2018
* Proposed solution for issue nim-lang#8919

* count sub/subs must be non-empty
narimiran pushed a commit that referenced this pull request Nov 1, 2018
* Proposed solution for issue #8919

* count sub/subs must be non-empty
narimiran pushed a commit that referenced this pull request Nov 1, 2018
* Proposed solution for issue #8919

* count sub/subs must be non-empty

(cherry picked from commit 14925ee)
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