command: Fix replace to be able to insert '$'

[JoeKar]

This will introduce at least the capability to insert $s, but I'm unsure, if every possible scenario can be fulfilled with that, because the user input can be complex, the expectation for the two different replace modes could be different and not everybody reads the documentation.

Fixes #2951

[JoeKar]

As already mentioned in #2951, it would be more common in case a regex search is used to implicit replace \ by \\ within the search string/argument, as we do it with this PR with the replace/template string/argument ($ -> $$).

[zyedidia]

I think we should only perform this replacement if noRegex is true. If the user is using regex, then they need to manually input $$ to escape dollar signs.

[JoeKar]

Correct me if I'm wrong, but in regex the character $ is escaped with \$ and not $$. The latter is a special thing of the replacement string aka template parameter of Regexp.Expand. It doesn't really rely on regex formatting.
This makes it currently a bit confusing in the usage aligned to the search string, which indeed is coupled to noRegex set or not.

[zyedidia]

Yes but the replaceStr is used with regexp.Expand and is not itself a regular expression so it shouldn't necessarily use \ to escape $. I agree it's unfortunately confusing.

[JoeKar]

So what you're basically referring to is to still keep the "template" functionality of the replaceStr in case we do a regex instead of a literal replace and therefore you request to do a move of the added into noRegex. Am I right?

But at this point we can be sure, that this will cause additional user/community issues created, because with a regex-search and a normal replace string (non-regex) you can't simply insert a $ character and you can't even escape it with \$. To argue this somehow weird/"unexpected" behavior can only be done by explaining the underlying used function and thus the then need usage of $$ instead of \$ or even $.

The current user base has already problems with the given search mechanism itself and this then remaining template functionality will cause further confusion. I've no problem to move that line, but I don't want to be the one explaining this over and over again in the issues. 😉

[Andriamanitra]

I think moving it inside the if noRegex is the right thing to do (in fact that's what I just did as I was fixing this issue for myself before I thought to check the issues/PRs).

The replace command is confusing for new users who don't realize it's using the full power of regex, but capture groups are a very useful feature for power users and it would be a shame to take it away without providing an alternative.

A better fix would be to completely remove regular expression functionality from replace/replaceall and make a separate regex-replace command that retains the full power of the feature including capture groups (related: #2963).

[JoeKar]

As already mentioned, it was never the intention to remove any functionality targeting regex/power users.
The only thing I was complaining about was the following fact:

But at this point we can be sure, that this will cause additional user/community issues created, because with a regex-search and a normal replace string (non-regex) you can't simply insert a $ character and you can't even escape it with \$.

Anyway, if you both insist that the correction shall be placed within the if noRegex { then I'm fine with that. It wouldn't become less confusing, since the Expand() is done implicit and not expected in the first place...even in case of non-literal, but I wont stay in the way of fixing the functionality in the literal replace.

Maybe it's worth to refer to Expand() in the documentation of replace.

[cben]

This won't say much to a user who doesn't speak Go. How about a summary like:

Suggested change

	In case the search is done non-literal (without `-l`) the character `$` can't
	be simply added, since `Regexp.Expand()` is used in the background to add
	more complex template replacement functionality. To add a `$` a double `$$`
	must be used.
	In case the search is done non-literal (without `-l`), the 'value'
	is interpreted as a template:
	* `$3` or `${3}` substitutes the submatch of the 3rd (capturing group)
	* `$foo` or `${foo}` substitutes the submatch of the (?P<foo>named group)
	* You have to write `$$` to substitute a literal dollar.

I don't think we need to elaborate on exact lexing e.g. "$10 is equivalent to ${10}, not ${1}0" — if user needs to care about that they're better off just using ${foo}.

(Well, perhaps do mention "as a Go Regexp.Expand template:" — even if one don't speak Go, knowing it's an existing convention can be reassuring vs. "an ad-hoc thing of this editor".)

[cben]

P.S. while we're here, perhaps 'value' is not the best term 🤔

I'd actually say it'd be clearest to document 2 separate usage lines, like some man pages do:

replace 'regexp' 'template' '-a'?
replace 'literal_text' 'replacement' '-l'

but not sure it fits the doc's style, and it's hard to explain how the flags can be combined (is it '-l' '-a'? '-l -a'? '-la'?)

[Out of scope for this PR: perhaps the literal/regexp modes are sufficiently different that they deserve splitting a separate command, e.g. replacestring? Also provides some discoverability by tab completion? 🤔]

[JoeKar]

I'd actually say it'd be clearest to document 2 separate usage lines, like some man pages do:

Unfortunately the behavior isn't that common as expected and I think we should solve that in the first place than precising corner cases within the documentation.

perhaps the literal/regexp modes are sufficiently different that they deserve splitting a separate command

Many GUI editors treat them in the same way with a search and an replace mask, while you've to set a checkbox for regex capabilities.
I wouldn't separate them, but they need to behave as the user would expect and currently it's everything else than obvious.

My suggestion would be to prevent the usage of Expand in a literal search. Just do a plain replace. But this is then a task for #2963. 🤔

[dmaluka]

FWIW a more "proper" implementation (preventing usage of regexp.Expand() for non-regex): dmaluka@8ec3273

Although both implementations produce the same result.

[dustdfg]

I think after closing this PR you will need to close this too #2440

[JoeKar]

I think after closing this PR you will need to close this too #2440

I don't think so. It doesn't freeze or crash. But test it on your own and you will see that the result maybe doesn't match your expectation. 😉

[dmaluka]

Yep, #2440 is a different issue, independent of this PR, since it is about $ in the search pattern (regex), not in the string the search pattern is replaced with.