micro
micro copied to clipboard
zyedidia
command: Fix replace to be able to insert '$'
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
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 ($ -> $$).
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.
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.
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.
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. :wink:
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).
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.
FWIW a more "proper" implementation (preventing usage of regexp.Expand() for non-regex): https://github.com/dmaluka/micro/commit/8ec3273fdba85052ee717181bfabc3adb2e31f28
Although both implementations produce the same result.
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. :wink:
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.