Replace $& with the whole match

[marcelgerber]

Implements the same special replacement patterns as in the Mozilla Reference.
See this little conversation: #5840 (comment)

Includes Unit tests.

[njx]

Hi - sorry I haven't gotten to this, been busy with non-Brackets stuff (and the holidays :)) I might not be able to get to it until the next sprint.

[marcelgerber]

@njx Would you mind reviewing this? Are there any changes to make?

[njx]

Sorry, my time has been crunched. Let me find someone else who can review it next week.

[njx]

Unassigning so we can look at reassigning it next week

[marcelgerber]

Seems like you forgot to re-assign this @njx. Sorry, but I don't want it to be lost ;)

[njx]

No, haven't forgotten. We're trying to finish out the sprint (a bit late due to some issues with file watchers) and so we aren't picking up other pull requests right now. Hopefully we'll get back to it in a few days. Sorry!

[kkarlesk]

@lkcampbell Would you please take a look at this one?

[lkcampbell]

@SAplayer, the parsing code appears to be correct.

I guess one of the things I don't understand is why we have to write our own parsing code for the dollar sign combinations if the Javascript engine already knows how to parse these patterns. Is there a way we could just have Javascript do that work for us?

[lkcampbell]

I guess I should pose the question for @njx as well since it looks like both of you did a lot of work on the dollar sign fixes. Why are we parsing regexps that contain dollar signs manually if JavaScript already understands how to parse them?

[marcelgerber]

@njx Answered this question himself in #5840: #5840 (comment)

[njx]

Thanks for digging that up @SAplayer - I had forgotten the reason :)

[njx]

(But there might be some clever way to do it that I hadn't thought of... @lkcampbell if you come up with one, let us know :) )

[lkcampbell]

@njx, I think you guys are correct, we may be losing the context of the match that allows the replacement code to work. Still, I will play around with it a little more today and see if there is another approach.

If nothing else, we should come up with a good comment to document the solution so we don't keep asking this same question every time we look at the code. But until I understand the problem a bit better, I don't know exactly what the comment is going to be.

[marcelgerber]

Maybe something like this?

FUTURE: Maybe try to use the JavaScript replace() function to do all parsing logic.

[njx]

If we don't actually have an idea for how to do it, I think the comment should just explain why we think we can't - e.g. "Note that we can't just use the ordinary replace() function here, because the string has been extracted from the original text and so might be missing some context that the regexp matched."

[lkcampbell]

Sounds good to me for now. I haven't had any revelations in the last 24 hours. @SAplayer, can you commit @njx's suggested comment?

[marcelgerber]

Just added this comment. Hope everything is fine now.

[lkcampbell]

@SAplayer, there is a Travis build fail. Something about pressEnter() not being defined.

[lkcampbell]

It looks like 5cd2043 removed the pressEnter() function and that is breaking your code.

[marcelgerber]

@lkcampbell Just fixed the unit tests. Thanks for your hint!

[lkcampbell]

Looks good. Merging.