[CLOSED] Fixing issue with double dollars in replace string

[core-ai-bot]

Issue by MarcelGerber
Monday Nov 04, 2013 at 16:23 GMT
Originally opened as adobe/brackets#5840

---

I picked this idea from adobe/brackets#5772 (comment).
This should be exactly the same dollar-signs-in-replace-with-string behaviour as JavaScript itself haves.

I did a bit of testing and everything worked fine (for example $1, $$1, $$$1, $, $$).

FindReplace tests passing, too.

---

MarcelGerber included the following code: https://github.com/adobe/brackets/pull/5840/commits

[core-ai-bot]

Comment by njx
Wednesday Nov 06, 2013 at 01:42 GMT

---

Hey--don't have time right now to fully think through/test this, but a couple of notes:

• I believe "$$" should turn into a single "$" in the actual replace string (i.e., it acts like the first dollar is just escaping the second).
• Now that this function is getting more complex, it would be good to factor it out into a separate function rather than duplicating it in both places. (You'll need to patch the appropriate match array into that function since it will no longer be inside the closure.)
• A couple of things that are not new issues but that I realized while looking at the code:
  • If there is no entry in the match array corresponding to the given reference, we should leave the reference unchanged instead of replacing it with "undefined".
  • We should handle references with multiple digits ($10, $11, etc.), though that's obviously not super common.
  • Let's change the function parameters to be more meaningful than w, d, i (I know that was in the original code but we can make it better :))
• This is getting complex enough that I think we should add unit tests for it.

[core-ai-bot]

Comment by njx
Wednesday Nov 06, 2013 at 01:45 GMT

---

BTW, here's a useful reference for the JS behavior: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace - under "Specifying a string as a parameter".

(This made me think...Instead of doing our own replacement function, would it make more sense to just use the actual JS replace function, calling it on the previously matched string, passing it the regexp and the user's substitution string? That way we wouldn't have to write all this logic to simulate the JS behavior--we would just get it for free. The precede/follow substitutions wouldn't work, but we haven't implemented those anyway.)

[core-ai-bot]

Comment by MarcelGerber
Wednesday Nov 06, 2013 at 06:17 GMT

---

1. $$ gets $ already
2. I tried this already, but the function wasn't executed, but passed to CM. But maybe I will give another try.
3. I will try to implement these. Shouldn't be difficult.
4. I've never done this, but I will give it a try.

I don't know how you want to pass matches to the js replace function, but I won't say anything against.

[core-ai-bot]

Comment by MarcelGerber
Wednesday Nov 06, 2013 at 18:37 GMT

---

Sorry, not the right commit name ;) It should be Fixed a bug where subexpressions like $01 weren't accepted

[core-ai-bot]

Comment by MarcelGerber
Wednesday Nov 06, 2013 at 19:13 GMT

---

Just added unit tests, so this PR is completely (in my view).
As these are my first unit tests, I hope they are ok (but I think so).
I wondered why there are no unit tests for Replace All?

[core-ai-bot]

Comment by MarcelGerber
Wednesday Nov 06, 2013 at 21:34 GMT

---

@njx Even if we would use another method to replace with subexpressions, the unit tests could be used anyway.

[core-ai-bot]

Comment by njx
Thursday Nov 07, 2013 at 18:56 GMT

---

Finished re-review. Looks good, just a few nits.

It seems like it would be good to add at least one Replace All case (I probably should have done this when I added the original one)...although that's more complex since you'd have to mock clicking the button in the Replace All panel (or add an API to directly invoke the replace all function). If you feel like looking into that, that would be great, but if not I think it's fine to just do some basic manual testing around it since it's pretty clear now that the behavior in all the edge cases will be consistent.

Longer term, when we rewrite the find/replace stuff (which hopefully won't be too long from now), we'll be refactoroing out the actual editing code from the UI, so these can become true unit tests instead of interacting with UI elements.

[core-ai-bot]

Comment by njx
Thursday Nov 07, 2013 at 18:58 GMT

---

BTW, the other idea I mentioned (about just using replace() itself on the original matched text to handle the dollar substitution) won't work because of lookahead and other contextual patterns (since that matched text will no longer be in the context of the original text).

[core-ai-bot]

Comment by MarcelGerber
Thursday Nov 07, 2013 at 19:30 GMT

---

Just pushed fixes for these nits. I will try to add one or more test(s) for Replace All, so don't merge yet.

[core-ai-bot]

Comment by MarcelGerber
Thursday Nov 07, 2013 at 20:22 GMT

---

Just pushed some unit tests for "Replace All". Just say if these are too much 😀

We should add some other tests for this, but that isn't my job.

[core-ai-bot]

Comment by njx
Thursday Nov 07, 2013 at 22:27 GMT

---

Thanks for all the additional unit tests! But I'm getting one failure in the new "$0" test--it doesn't seem to be matching the expected text. Are you seeing the same failure?

[core-ai-bot]

Comment by MarcelGerber
Friday Nov 08, 2013 at 05:29 GMT

---

No, for me every test is working. Which error message do you get?

[core-ai-bot]

Comment by njx
Friday Nov 08, 2013 at 18:59 GMT

---

Hmmm, it's working fine for me now--not sure what was wrong. Thanks for working on this! Merging.

[core-ai-bot]

Comment by MarcelGerber
Friday Nov 08, 2013 at 19:21 GMT

---

Thank you. You should add unit tests for normal "Replace All" cases.

[core-ai-bot]

Comment by MarcelGerber
Saturday Nov 09, 2013 at 10:19 GMT

---

@njx I would also like to add the functionality of $&, $ and $' (in another PR). (see [here](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace)) $& is easy (just match[0]), but in the case of $ should we use the whole text that is before the match or should we just use the beginning of the line (and then, should we include leading whitespace or not?)

[core-ai-bot]

Comment by MarcelGerber
Saturday Nov 09, 2013 at 23:10 GMT

---

Or maybe we shouldn't include $` and $' at all, just $&?

[core-ai-bot]

Comment by njx
Sunday Nov 10, 2013 at 02:05 GMT

---

It would make sense to add $& (though not really critical IMO). I don't really know about $` or $'...like you said their meaning is somewhat ambiguous in our context and I don't know how commonly they would be used anyway.

[core-ai-bot]

Comment by MarcelGerber
Sunday Nov 10, 2013 at 10:59 GMT

---

Okay, I don't know what to use them for, too.
But $& can be helpful. I will add it soon.

[core-ai-bot]

Comment by MarcelGerber
Friday Jan 31, 2014 at 14:59 GMT

---

Just pushed some unit tests for "Replace All". Just say if these are too much
We should add some other tests for this, but that isn't my job.

@njx Would you mind adding some more, general unit tests for Replace All?

[core-ai-bot]

Comment by njx
Friday Jan 31, 2014 at 17:28 GMT

---

We have an open card for adding real unit tests for Find/Replace, so that should get done soon. I wouldn't worry about it for this PR as long as your own cases are tested.