Rework "analysis.updateContent" analysis server request.

pkg/analysis_server/lib/src/domain_analysis.dart

 // Copyright (c) 2014, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 library domain.analysis;
 
 import 'dart:collection';
 
 import 'package:analysis_server/src/analysis_server.dart';
 import 'package:analysis_server/src/computer/computer_hover.dart';
@@ skipping 153 matching lines @@
     server.setAnalysisSubscriptions(subMap);
     return new Response(request.id);
   }
 
   /**
    * Implement the 'analysis.updateContent' request.
    */
   Response updateContent(Request request) {
     var changes = new HashMap<String, ContentChange>();
     RequestDatum filesDatum = request.getRequiredParameter(FILES);
+    Response errorResponse;
     filesDatum.forEachMap((file, changeDatum) {
-      var change = new ContentChange();
-      change.content = changeDatum[CONTENT].isNull ?
-          null :
-          changeDatum[CONTENT].asString();
-      if (changeDatum.hasKey(OFFSET)) {
-        change.offset = changeDatum[OFFSET].asInt();
-        change.oldLength = changeDatum[OLD_LENGTH].asInt();
-        change.newLength = changeDatum[NEW_LENGTH].asInt();
+      ContentChange change = new ContentChange();
+      switch (changeDatum[TYPE].asString()) {
+        case 'add':
+          change.content = changeDatum[CONTENT].asString();
+          break;
+        case 'change':
+          change.offset = changeDatum[OFFSET].asInt();
+          change.oldLength = changeDatum[OLD_LENGTH].asInt();
+          change.content = changeDatum[REPLACEMENT].asString();
+          break;
+        case 'remove':
+          break;
+        default:
+          errorResponse = new Response.invalidParameter(request,
+              changeDatum[TYPE].path, 'be one of "add", "change", or "remove"');
+          return;

[Brian Wilkerson, 2014/08/07 22:15:22]

I think we want to return the errorResponse at this point.

[Paul Berry, 2014/08/07 23:58:09]

Unfortunately, that doesn't work, because we are inside a callback passed to
foreachMap(), so the return won't return all the way out of updateContent().

Would you prefer the following?

  for (String file in filesDatum.keys) {
    RequestDatum changeDatum = filesDatum[file];
    ContentChange change = new ContentChange();
    switch (...) {
      ...
      default:
        return new Response.invalidParameter(...);
    }
    changes[file] = change;
  }

[Brian Wilkerson, 2014/08/08 00:14:58]

What I want is non-local returns.

What I need is to see when there's a closure so that I don't miss the fact that
we're running in a different context.

I'm not sure whether the suggested code would be better or not. I leave it to
your judgement.

       }
       changes[file] = change;
     });
+    if (errorResponse != null) {

[Brian Wilkerson, 2014/08/07 22:15:22]

In which case errorResponse will never be non-null here.

+      return errorResponse;
+    }
     server.updateContent(changes);
     return new Response(request.id);
   }
 
   /**
    * Implement the 'analysis.updateOptions' request.
    */
   Response updateOptions(Request request) {
     // options
     RequestDatum optionsDatum = request.getRequiredParameter(OPTIONS);
@@ skipping 44 matching lines @@
     server.updateOptions(updaters);
     return new Response(request.id);
   }
 }
 
 
 /**
  * A description of the change to the content of a file.
  */
 class ContentChange {
   String content;

[Brian Wilkerson, 2014/08/07 22:15:22]

Perhaps change 'content' to 'replacement'?

[Paul Berry, 2014/08/07 23:58:09]

I'm not sure that would be an improvement, because unfortunately this field is
now serving a dual role: if the change is an AddContentOverlay, then it is the
full file contents; if the change is a ChangeContentOverlay, then it is the
replacement text. :(

How about this?

  /**
   * If [offset] and [length] are null, the full content of the file (or null
   * if the file should be read from the filesystem).
   * 
   * If [offset] and [length] are non-null, the replacement text which should
   * take the place of the [length] characters of the file starting at [offset].
   */
  String contentOrReplacement;

[Brian Wilkerson, 2014/08/08 00:14:58]

A bit wordy, but I would have understood what the field actually holds. You can
decide.

   int offset;
   int oldLength;

[Brian Wilkerson, 2014/08/07 22:15:22]

Perhaps rename 'oldLength' to 'length'?

[Paul Berry, 2014/08/07 23:58:09]

Done.

-  int newLength;
 }