Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

REVIEW ONLY: Fix regexp replace in Replace All #5772

Merged
merged 1 commit into from
Oct 30, 2013
Merged

REVIEW ONLY: Fix regexp replace in Replace All #5772

merged 1 commit into from
Oct 30, 2013

Conversation

njx
Copy link

@njx njx commented Oct 30, 2013

For #5770.

When doing a replace all, the replace function used by regexp replace needs to use the matches from each item in the
replace all, not the match that was found in the calling replace().

@marcelgerber
Copy link
Contributor

When there are only 3 params left, we should call the function with only 3 params, too.
If you'd fix that, I'd prefer your PR.

@gruehle
Copy link
Member

gruehle commented Oct 30, 2013

Everything looks good to me. I did a bunch of testing with 'Replace All', and everything worked as expected.

@peterflynn
Copy link
Member

@njx Fix looks good to me too. It doesn't look like there's any way it could affect the existing non-regexp Replace All functionality.

OTOH I think it'd also be ok to just change the release notes to say $n substitution is only supported for regular Replace.

@njx
Copy link
Author

njx commented Oct 30, 2013

@SAplayer - not sure what you mean. I removed the old replace function from the call to _showReplaceAllPanel().

@peterflynn
Copy link
Member

Btw -- not related to this bug, but... if the user wanted the literal string "$1" to be the replacement when doing a regexp Replace, is there any escaping that would allow that? It looks like you'd normally be able to use "$$" to escape "$," but we don't appear to support that.

@marcelgerber
Copy link
Contributor

@njx I mean, when this function is called, we should only pass 3 params instead of 4.

@njx
Copy link
Author

njx commented Oct 30, 2013

@SAplayer Can you comment on the specific line you're referring to? The only call to that function is 3 params: _showReplaceAllPanel(editor, query, text); on line 555.

@njx
Copy link
Author

njx commented Oct 30, 2013

@peterflynn Good point, should file that separately.

@marcelgerber
Copy link
Contributor

@njx Oh, sorry, I've missed that. Everything's ok ;)

njx pushed a commit that referenced this pull request Oct 30, 2013
REVIEW ONLY: Fix regexp replace in Replace All
@njx njx merged commit 5785df6 into master Oct 30, 2013
@njx njx deleted the nj/issue-5770 branch October 30, 2013 23:09
@njx
Copy link
Author

njx commented Nov 8, 2013

@peterflynn - @SAplayer added code to handle $$; if you have a chance, feel free to try it and see if you find any issues.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants