-
Notifications
You must be signed in to change notification settings - Fork 55
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
add str::substring #158
add str::substring #158
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is nice! I made some comments in the changed files, it would be good to fix these before merging.
@ISibboI , the usize overflow was already handled, but I changed the behavior to raise an error instead of returning an empty string when supplied indices are out of bounds. Let me know if that's ok :) |
assert!(eval("str::substring(\"foobar\", 2, 1)").is_err()); | ||
assert!(eval("str::substring(\"foobar\", 99999)").is_err()); | ||
assert!(eval("str::substring(\"foobar\", -1)").is_err()); | ||
assert!(eval("str::substring(\"foobar\", 0, -1)").is_err()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think currently, the function accepts also the case where it gets four or more arguments, and silently drops them. Let's add a test case for that, expecting an error. And then if the function does not return an error in that case, let's make it return one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes so far! I think this is going nicely. I noticed one more edge case that we overlooked, see the comment.
Also, in case I get to implement #159 before merging this, it may be good to use it as well. But I can also do that. |
previously, the function just returned an empty string
Yeah, now you can use |
Okay, I'm leaving this in your hands. thanks ! |
Thank you 🙏 |
No description provided.