Refactor to avoid breaking change; prompt the user instead of pass do…

…wn option; add tests and fixtures

lib/src/run_interactive_codemod.dart

@@ -80,7 +80,6 @@ int runInteractiveCodemod(
   bool defaultYes = false,
   String additionalHelpOutput,
   String changesRequiredOutput,
-  bool skipOverlaps = false,
 }) =>
     runInteractiveCodemodSequence(
       query,
@@ -89,7 +88,6 @@ int runInteractiveCodemod(
       defaultYes: defaultYes,
       additionalHelpOutput: additionalHelpOutput,
       changesRequiredOutput: changesRequiredOutput,
-      skipOverlaps: skipOverlaps,
     );
 
 /// Exactly the same as [runInteractiveCodemod] except that it runs all of the
@@ -116,7 +114,6 @@ int runInteractiveCodemodSequence(
   bool defaultYes = false,
   String additionalHelpOutput,
   String changesRequiredOutput,
-  bool skipOverlaps = false,
 }) {
   try {
     ArgResults parsedArgs;
@@ -148,8 +145,7 @@ int runInteractiveCodemodSequence(
         stdout.supportsAnsiEscapes,
         () => _runInteractiveCodemod(query, suggestors, parsedArgs,
             defaultYes: defaultYes,
-            changesRequiredOutput: changesRequiredOutput,
-            skipOverlaps: skipOverlaps));
+            changesRequiredOutput: changesRequiredOutput));
   } catch (error, stackTrace) {
     stderr..writeln('Uncaught exception:')..writeln(error)..writeln(stackTrace);
     return ExitCode.software.code;
@@ -189,9 +185,7 @@ final codemodArgParser = ArgParser()
 
 int _runInteractiveCodemod(
     FileQuery query, Iterable<Suggestor> suggestors, ArgResults parsedArgs,
-    {bool defaultYes,
-    String changesRequiredOutput,
-    bool skipOverlaps = false}) {
+    {bool defaultYes, String changesRequiredOutput}) {
   final failOnChanges = parsedArgs['fail-on-changes'] ?? false;
   final stderrAssumeTty = parsedArgs['stderr-assume-tty'] ?? false;
   final verbose = parsedArgs['verbose'] ?? false;
@@ -213,10 +207,7 @@ int _runInteractiveCodemod(
     return ExitCode.noInput.code;
   }
 
-  // Used to collect information about which patches are skipped throughout the
-  // entire codemod.
-  final patchCollector = PatchCollector();
-
+  List<Patch> skippedPatches = [];
   stdout.writeln('searching...');
   for (final suggestor in suggestors) {
     final filePaths = query.generateFilePaths().toList()..sort();
@@ -299,8 +290,17 @@ int _runInteractiveCodemod(
           }
           if (choice == 'q') {
             logger.fine('applying patches');
-            applyPatchesAndSave(sourceFile, appliedPatches, patchCollector,
-                skipOverlaps: skipOverlaps);
+            var userSkipped = promptToHandleOverlappingPatches(appliedPatches);
+            // Add patches to global list to print info about skipped patches
+            // after running the codemod.
+            skippedPatches.addAll(userSkipped);
+
+            // Don't apply the patches the user skipped.
+            for (var patch in userSkipped) {
+              appliedPatches.remove(patch);
+            }
+
+            applyPatchesAndSave(sourceFile, appliedPatches);
             logger.fine('quitting');
             return ExitCode.success.code;
           }
@@ -313,15 +313,26 @@ int _runInteractiveCodemod(
 
       if (!failOnChanges) {
         logger.fine('applying patches');
-        applyPatchesAndSave(sourceFile, appliedPatches, patchCollector,
-            skipOverlaps: skipOverlaps);
+
+        var userSkipped = promptToHandleOverlappingPatches(appliedPatches);
+        // Add patches to global list to print info about skipped patches
+        // after running the codemod.
+        skippedPatches.addAll(userSkipped);
+
+        // Don't apply the patches the user skipped.
+        for (var patch in userSkipped) {
+          appliedPatches.remove(patch);
+          logger.fine('skipping patch ${patch}');
+        }
+
+        applyPatchesAndSave(sourceFile, appliedPatches);
       }
     }
   }
   logger.fine('done');
 
-  if (patchCollector.skippedPatches.isNotEmpty) {
-    for (var patch in patchCollector.skippedPatches) {
+  if (skippedPatches.isNotEmpty) {
+    for (var patch in skippedPatches) {
       stdout.writeln(
           'NOTE: Overlapping patch was skipped. May require manual modification.');
       stdout.writeln('      ${patch.toString()}');

lib/src/util.dart

@@ -23,35 +23,21 @@ import 'constants.dart';
 import 'file_query.dart';
 import 'patch.dart';
 
-/// Collector whose instance is passed to every source file to aggregate patches
-/// from the entire code modification.
-class PatchCollector {
-  List<Patch> skippedPatches = [];
-}
-
 /// Returns the result of applying all of the [patches]
 /// (insertions/deletions/replacements) to the contents of [sourceFile].
 ///
 /// Throws an [Exception] if any two of the given [patches] overlap and the
 /// `skipOverlaps` argument is false or not provided. If `skipOverlaps` is true,
 /// overlapping patches will be added to the [PatchCollector] so that
 /// after the code modification completes.
-String applyPatches(SourceFile sourceFile, Iterable<Patch> patches,
-    PatchCollector patchCollector,
-    {bool skipOverlaps = false}) {
+String applyPatches(SourceFile sourceFile, Iterable<Patch> patches) {
   final buffer = StringBuffer();
   final sortedPatches = patches.toList()..sort();
 
   var lastEdgeOffset = 0;
   for (final patch in sortedPatches) {
     if (patch.startOffset < lastEdgeOffset) {
-      if (!skipOverlaps) {
-        throw new Exception('Overlapping patch is not allowed.');
-      } else {
-        stdout.writeln('Skipping overlapping patch ${patch.toString()}');
-        patchCollector.skippedPatches.add(patch);
-        continue;
-      }
+      throw new Exception('Overlapping patch is not allowed.');
     }
 
     // Write unmodified text from end of last patch to beginning of this patch
@@ -77,20 +63,54 @@ String applyPatches(SourceFile sourceFile, Iterable<Patch> patches,
 /// Throws an [ArgumentError] if [sourceFile] has a null value for
 /// [SourceFile.url], as it is required to open the file and write the new
 /// contents.
-void applyPatchesAndSave(SourceFile sourceFile, Iterable<Patch> patches,
-    PatchCollector patchCollector,
-    {bool skipOverlaps = false}) {
+void applyPatchesAndSave(SourceFile sourceFile, Iterable<Patch> patches) {
   if (patches.isEmpty) {
     return;
   }
   if (sourceFile.url == null) {
     throw new ArgumentError('sourceFile.url cannot be null');
   }
-  final updatedContents = applyPatches(sourceFile, patches, patchCollector,
-      skipOverlaps: skipOverlaps);
+  final updatedContents = applyPatches(sourceFile, patches);
   File(sourceFile.url.path).writeAsStringSync(updatedContents);
 }
 
+/// Finds overlapping patches and prompts the user to decide how to handle them.
+///
+/// The user can either skip the patch and continue running the codemod, or
+/// terminate the codemod.
+List<Patch> promptToHandleOverlappingPatches(Iterable<Patch> patches) {
+  List<Patch> skippedPatches = [];
+  final sortedPatches = patches.toList()..sort();
+
+  var lastEdgeOffset = 0;
+  for (final patch in sortedPatches) {
+    if (patch.startOffset < lastEdgeOffset) {
+      stdout.writeln(
+          'A patch that overlaps with a previous patch applied was found. '
+          'Do you want to skip this patch, or quit the codemod?\n'
+          'Overlapping patch: ${patch.toString()}\n'
+          'Updated text: ${patch.updatedText}\n'
+          '(s = skip this patch and apply the rest [default],\n'
+          'q = quit)');
+
+      var choice = prompt('sq', 's');
+
+      if (choice == 's') {
+        skippedPatches.add(patch);
+      }
+
+      if (choice == 'q') {
+        throw new Exception(
+            'User terminated codemod due to overlapping patch.\n'
+            'Overlapping patch: ${patch.toString()}\n'
+            'Updated text: ${patch.updatedText}\n');
+      }
+    }
+    lastEdgeOffset = patch.endOffset;
+  }
+  return skippedPatches;
+}
+
 /// Returns the number of lines that a patch diff should be constrained to.
 /// Based on the stdout terminal size if available, or a sane default if not.
 int calculateDiffSize(Stdout stdout) {

test/functional/run_interactive_codemod_test.dart

@@ -31,6 +31,9 @@ const _testFixturesPath = 'test_fixtures/functional';
 const _afterAllPatches = '$_testFixturesPath/after_all_patches/';
 const _afterSomePatches = '$_testFixturesPath/after_some_patches/';
 const _afterNoPatches = '$_testFixturesPath/before/';
+const _overlappingPatchesQuit = '$_testFixturesPath/before/';
+const _overlappingPatchesSkip =
+    '$_testFixturesPath/after_overlapping_patches_skip/';
 const _projectPath = '$_testFixturesPath/before/';
 
 Future<Null> testCodemod(
@@ -179,5 +182,38 @@ void main() {
         script: 'codemod_changes_required_output.dart', body: (out, err) {
       expect(err, contains('6 change(s) needed.\n\nchanges required output'));
     });
+
+    testCodemod(
+        'skips overlapping patches via prompts', _overlappingPatchesSkip,
+        expectedExitCode: 0,
+        stdinLines: ['y', 'y', 's', 'y', 'y', 's', 'n', 'n'],
+        script: 'codemod_overlapping_patches.dart', body: (out, err) {
+      expect(
+          out,
+          contains(
+              'NOTE: Overlapping patch was skipped. May require manual modification.\n'
+              '      <Patch: on file1.txt from 1:2 to 1:4>\n'
+              '      Updated text:\n'
+              '      overlap\n'
+              '\n'
+              'NOTE: Overlapping patch was skipped. May require manual modification.\n'
+              '      <Patch: on file2.txt from 1:2 to 1:4>\n'
+              '      Updated text:\n'
+              '      overlap\n'
+              '\n'));
+    });
+
+    testCodemod('quits codemod via prompts when overlapping patches',
+        _overlappingPatchesQuit,
+        expectedExitCode: 70,
+        stdinLines: ['y', 'y', 'q'],
+        script: 'codemod_overlapping_patches.dart', body: (out, err) {
+      expect(
+          err,
+          contains(
+              'Exception: User terminated codemod due to overlapping patch.\n'
+              'Overlapping patch: <Patch: on file1.txt from 1:2 to 1:4>\n'
+              'Updated text: overlap\n'));
+    });
   });
 }

test_fixtures/functional/after_overlapping_patches_skip/file1.txt

@@ -0,0 +1,3 @@
+dove 1
+line 2
+line 3

test_fixtures/functional/after_overlapping_patches_skip/file2.txt

@@ -0,0 +1,3 @@
+dove 1
+line 2
+line 3

test_fixtures/functional/after_overlapping_patches_skip/skip.txt

@@ -0,0 +1 @@
+skip

test_fixtures/functional/before/codemod_overlapping_patches.dart

@@ -0,0 +1,19 @@
+import 'dart:io';
+
+import 'package:codemod/codemod.dart';
+import 'package:source_span/source_span.dart';
+
+void main(List<String> args) {
+  exitCode = runInteractiveCodemod(FileQuery.dir(pathFilter: (path) => path.endsWith('.txt')), OverlappingPatchSuggestor(), args: args);
+}
+
+class OverlappingPatchSuggestor implements Suggestor {
+  @override
+  bool shouldSkip(String sourceFileContents) => false;
+
+  @override
+  Iterable<Patch> generatePatches(SourceFile sourceFile) sync* {
+    yield Patch(sourceFile, sourceFile.span(0, 3), 'dov');
+    yield Patch(sourceFile, sourceFile.span(1, 3), 'overlap');
+  }
+}