Fixing issue with double dollars in replace string #5840

marcelgerber · 2013-11-04T16:23:23Z

I picked this idea from #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.

behaves like javascript itself

njx · 2013-11-06T01:42:58Z

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.

njx · 2013-11-06T01:45:32Z

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.)

marcelgerber · 2013-11-06T06:17:11Z

$$ gets $ already
I tried this already, but the function wasn't executed, but passed to CM. But maybe I will give another try.
I will try to implement these. Shouldn't be difficult.
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.

…/sh.exe"1 weren't accepted

marcelgerber · 2013-11-06T18:37:21Z

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

marcelgerber · 2013-11-06T19:13:09Z

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?

marcelgerber · 2013-11-06T21:34:49Z

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

njx · 2013-11-07T18:32:53Z

src/search/FindReplace.js

@@ -117,6 +117,12 @@ define(function (require, exports, module) {
        }
    }

+    function parseDollars(replaceWith, match) {
+        replaceWith = replaceWith.replace(/(\$+)(\d{1,2})/g, function (whole, dollars, index) { return dollars.length % 2 === 1 ? dollars.substr(1) + ((match.hasOwnProperty("result") ? match.result[parseInt(index, 10)] : match[parseInt(index, 10)]) || "") : dollars + index; });


It would be cleaner and easier to understand if you just pass in match.result from the replaceAll case instead of having to deal with both cases here.

Seems like the "else" case could just be whole instead of dollars + index.

After those fixes, it might still be okay for this to be on a single line, but it might be nicer to just break it out into separate lines anyway.

Yes, it would be good if there were several lines, but I dunno where to break and how to indent.

njx · 2013-11-07T18:56:59Z

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.

njx · 2013-11-07T18:58:57Z

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).

marcelgerber · 2013-11-07T19:30:53Z

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

marcelgerber · 2013-11-07T20:22:02Z

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 · 2013-11-07T22:17:22Z

src/search/FindReplace.js

+        });
+        // we need to escape (\$) these chars in the regexp to avoid to get these parsed as
+        // normal regexp anchors
+        replaceWith = replaceWith.replace(/\$\$/g, "$$");


Actually, my comment here was about the "$$" in the replacement string being confusing (because it looks like you're replacing "$$" with "$$"--but the replacement "$$" is really just an escaped "$"), not the backslashes in the regexp. Like I mentioned before, I don't think you need the double-dollar in the replacement string since it won't be interpreted as a metacharacter there if it doesn't have anything after it. But this isn't a big deal.

njx · 2013-11-07T22:27:46Z

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?

marcelgerber · 2013-11-08T05:29:38Z

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

njx · 2013-11-08T18:59:29Z

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

Fixing issue with double dollars in replace string

marcelgerber · 2013-11-08T19:21:07Z

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

marcelgerber · 2013-11-09T10:19:05Z

@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?)

marcelgerber · 2013-11-09T23:10:26Z

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

njx · 2013-11-10T02:05:23Z

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.

marcelgerber · 2013-11-10T10:59:58Z

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

marcelgerber · 2014-01-31T14:59:29Z

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?

njx · 2014-01-31T17:28:28Z

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.

Fixing issue with double dollars in replace string

d800944

behaves like javascript itself

ghost assigned njx Nov 4, 2013

Marcel Gerber added 3 commits November 6, 2013 16:57

Implementing ideas based on nj's feedback

54b1a1a

Implementing function parseDollars

f449db9

Fixed a bug where subexpressions like "C:/Program Files (x86)/Git/bin…

9457a94

…/sh.exe"1 weren't accepted

Adding unit tests

8ce9095

njx reviewed Nov 7, 2013
View reviewed changes

Marcel Gerber added 2 commits November 7, 2013 20:27

Fixing nits based on nj's feedback

94b0d81

Adding test to ensure correct behaviour with $0

6c9e49e

Marcel Gerber added 2 commits November 7, 2013 20:36

Adding comment to explain why we escape $ in the regexp

2802ff7

Adding some unit tests for "Replace All"

3713a20

njx reviewed Nov 7, 2013
View reviewed changes

Changing unnecessary $$, removing comment

3d3a3d8

njx pushed a commit that referenced this pull request Nov 8, 2013

Merge pull request #5840 from SAPlayer/replace-double-dollar

08eb10b

Fixing issue with double dollars in replace string

njx merged commit 08eb10b into adobe:master Nov 8, 2013

marcelgerber deleted the replace-double-dollar branch November 8, 2013 19:18

marcelgerber mentioned this pull request Nov 10, 2013

Replace $& with the whole match #5929

Merged

This was referenced Aug 30, 2021

[CLOSED] Fixing issue with double dollars in replace string brackets-archive/bracketsIssues#5348

Open

[CLOSED] Replace $& with the whole match brackets-archive/bracketsIssues#5416

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing issue with double dollars in replace string #5840

Fixing issue with double dollars in replace string #5840

marcelgerber commented Nov 4, 2013

njx commented Nov 6, 2013

njx commented Nov 6, 2013

marcelgerber commented Nov 6, 2013

marcelgerber commented Nov 6, 2013

marcelgerber commented Nov 6, 2013

marcelgerber commented Nov 6, 2013

njx Nov 7, 2013

njx Nov 7, 2013

njx Nov 7, 2013

marcelgerber Nov 7, 2013

njx commented Nov 7, 2013

njx commented Nov 7, 2013

marcelgerber commented Nov 7, 2013

marcelgerber commented Nov 7, 2013

njx Nov 7, 2013

njx commented Nov 7, 2013

marcelgerber commented Nov 8, 2013

njx commented Nov 8, 2013

marcelgerber commented Nov 8, 2013

marcelgerber commented Nov 9, 2013

marcelgerber commented Nov 9, 2013

njx commented Nov 10, 2013

marcelgerber commented Nov 10, 2013

marcelgerber commented Jan 31, 2014

njx commented Jan 31, 2014

Fixing issue with double dollars in replace string #5840

Fixing issue with double dollars in replace string #5840

Conversation

marcelgerber commented Nov 4, 2013

njx commented Nov 6, 2013

njx commented Nov 6, 2013

marcelgerber commented Nov 6, 2013

marcelgerber commented Nov 6, 2013

marcelgerber commented Nov 6, 2013

marcelgerber commented Nov 6, 2013

njx Nov 7, 2013

Choose a reason for hiding this comment

njx Nov 7, 2013

Choose a reason for hiding this comment

njx Nov 7, 2013

Choose a reason for hiding this comment

marcelgerber Nov 7, 2013

Choose a reason for hiding this comment

njx commented Nov 7, 2013

njx commented Nov 7, 2013

marcelgerber commented Nov 7, 2013

marcelgerber commented Nov 7, 2013

njx Nov 7, 2013

Choose a reason for hiding this comment

njx commented Nov 7, 2013

marcelgerber commented Nov 8, 2013

njx commented Nov 8, 2013

marcelgerber commented Nov 8, 2013

marcelgerber commented Nov 9, 2013

marcelgerber commented Nov 9, 2013

njx commented Nov 10, 2013

marcelgerber commented Nov 10, 2013

marcelgerber commented Jan 31, 2014

njx commented Jan 31, 2014