Skip to next match when user doesn't change

[steven807]

If the user selects 'n' (to skip a match), the next comparison only starts one character later. If the beginning of the patch pattern, is, e.g. '( +)' this may continue to find the same match many times. I've added logic that starts the search from the end of the previous match, if the user skips the last change.
This is not very well tested, so feel free to review it.

[swolchok]

Hmm, I'm not sure whether or not the intent of this change is something we should try to do. On the one hand, the current implementation matches the original codemod tool's behavior when codemod in -m mode: https://github.com/facebook/codemod/blob/master/codemod/base.py#L164-L191
On the other hand, when it's not in -m mode, codemod suggests an entire line at a time. If your file is "foo foo foo foo" and you do codemod foo bar, codemod will present you with one suggestion, which is to replace the entire file with "bar bar bar bar", whereas fastmod will ask you once for each "foo". So, we already don't mirror codemod perfectly, and I'm OK with not being exactly the same.

The thing I'm worried about is that there may be cases where you really do want to go to the next submatch of your regex. I haven't been able to construct a realistic-looking example, though. (I thought that something like fastmod 'foo\((.*)\)' 'bar(${1})' on foo(baz(), foo()) where you decided that you only wanted to replace the inner call to foo() would be a good example, but the first match would be foo(baz(), and not being able to match parens is a fundamental limitation of DFA-based regex matching.) I'd like to think it over (and solicit input from the community!) for a bit. Thanks for raising this issue!

[swolchok]

First of all, it's been a while since this PR came out and we've since made at least 1 change to the advancement logic, so the conflicts are real and it definitely can't be merged as-is. (Sorry for the delay! It took a while to flush out other issues and then ruminate on them.)

Second, I want to fix all our issues with advancement at once rather than doing one case at a time. We need an advancement policy for each of the actions the user can take:

• accept replacement (y)
• reject replacement and continue (n)
• open editor (e)
• accept all replacements (A)
• apply replacement and open editor (E)

I'll just talk through each of these cases below, keeping fastmod's philosophy in mind:

fastmod's major philosophical difference from codemod is that it is focused on improving the use case "I want to use interactive mode to make sure my regex is correct, and then I want to apply the regex everywhere"

Accept replacement (y)

Here, our current policy is to advance 0 characters if the replacement has length 0 and 1 character otherwise. We've already rejected the policy "advance 1 character always" (see 501a8b9 and 95ae97a). Advancing 1 character has the advantage that it allows you to do further editing on replacement text, but given fastmod's philosophy of driving toward automated application of your regex everywhere, this advantage has minimal value.

Proposed new policy: advance to the next character after the end of the replacement text.

Reject replacement (n)

We have two policies to choose from when a replacement is rejected: 1) We can advance 1 character and try again (existing behavior), or 2) we can advance to the end of the regex match and try again (@steven807's proposal in this PR). I think the answer to the tradeoff has to come from fastmod's philosophy.

If you say n at fastmod's interactive prompt without quitting, you're already outside our core use case. We're left trying to guess if your intent is "hmm, my regex looks wrong, but I want to keep searching to make sure I continue to see nonsense" (which would suggest policy 2) or "I do want to do this replacement, but the start position is wrong, so let's manually move over 1 character and try again" (which would suggest policy 1). Given our philosophy, I think @steven807 is right that we should move to policy 2 in the rejection case.

Proposed new policy: advance to the next character after the end of the match.

Open editor (e)

Our current policy is to advance 1 character and keep going.

The fundamental problem with the open editor option is that the user could arbitrarily change the document if they wanted to, and since this is outside our core use case, we're never going to have a great picture of their change. I propose that we should assume that the user is "working with us" by only editing within the matched region, may or may not have actually applied any changes, and wants to continue to the next match when they start interacting with fastmod again. Given these assumptions, I think our existing policy makes sense.

Proposed new policy: no change.

Apply replacement and open editor (E)

Our current policy is to advance 1 character and keep going. Since interactive editing is not our core use case, I think we should not worry about the difference between E and e.

Proposed new policy: no change.

Accept all replacements (A)

Our current policy is to act as though the user pressed y repeatedly. There is an infinite loop bug with A on master for both codemod and fastmod:

$ echo 'foo foo' > /tmp/test.txt
$ cd /tmp
$ codemod -m foo barfoo --extension txt
press A to accept all interactively

The problem there is that the replacement matches the search pattern, so we continually replace our own replacement (and grow it).

However, given the proposed policy changes for y, I think we can keep the policy for A as-is.

Proposed new policy: no direct change (act as though y was pressed at every subsequent prompt), but inherit the changes in the y policy.

Next steps

The next step is to implement the new policy. @steven807, if you want to do that, that's great, but I totally understand if the moment has passed.

[steven807]

Yup, I'm afraid it has passed. I'll be looking forward to see how things progress, though!

[facebook-github-bot]

This pull request has been merged in 767f874.