Rework "analysis.updateContent" analysis server request.

pkg/analysis_server/lib/src/domain_analysis.dart

Index: pkg/analysis_server/lib/src/domain_analysis.dart
diff --git a/pkg/analysis_server/lib/src/domain_analysis.dart b/pkg/analysis_server/lib/src/domain_analysis.dart
index ed21d0af1f60e16b3813063d9b44463065c64897..8c1252bdef8e6ac581fe26236a42de59e6800104 100644
--- a/pkg/analysis_server/lib/src/domain_analysis.dart
+++ b/pkg/analysis_server/lib/src/domain_analysis.dart
@@ -171,18 +171,30 @@ class AnalysisDomainHandler implements RequestHandler {
   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);
   }
@@ -250,5 +262,4 @@ 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;
 }