-
Notifications
You must be signed in to change notification settings - Fork 129
Added string.replace() and .replaceAll() #698
Conversation
hkubus
commented
Jul 3, 2024
@microsoft-github-policy-service agree
@pelikhan
pelikhan
left a comment
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.
Could you add a test with replacing a string with multiple characters?
hkubus
commented
Jul 3, 2024
Could you add a test with replacing a string with multiple characters?
Added, also found a bug where replaceAll could loop infinitely so fixed that as well.
pelikhan
commented
Jul 3, 2024
what if searchvalue equals replace value?
hkubus
commented
Jul 3, 2024
i believe it shouldn't be an issue, that was part of the bug with replaceAll because if replaceValue contained searchValue it would just match against it forever.
Now, after replacing a match it only searches for the index after the position of the replacement value so it can't possibly match against the thing it just replaced. Should i add that as a test just in case anyways?
pelikhan
commented
Jul 4, 2024
What if you replace "" with ""?
finishes 1 task off of #683