Move Location factories and RefactoringProblemSeverity.max into the generated classes.

pkg/analysis_server/lib/src/generated_protocol.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.
 //
 // This file has been automatically generated.  Please do not edit it manually.
 // To regenerate the file, use the script
 // "pkg/analysis_server/spec/generate_files".
 
 part of protocol2;
 
@@ skipping 6604 matching lines @@
       } else {
         throw jsonDecoder.missingKey(jsonPath, "startColumn");
       }
       return new Location(file, offset, length, startLine, startColumn);
     } else {
       throw jsonDecoder.mismatch(jsonPath, "Location");
     }
   }
 
   /**
-   * Construct based on an element from the analyzer engine.
+   * Create a Location based on an [engine.Element].
    */
-  factory Location.fromElement(engine.Element element) =>
-      _locationFromElement(element);
+  factory Location.forElement(engine.Element element) =>

[Paul Berry, 2014/08/22 16:46:59]

Can you help me understand the motivation for this name change?  To me, "new
Location.fromElement(e)" means "take the information in element e, and create a
new Location object based on it".  Which is I think what this method does.

Whereas "new Location.forElement(e)" confuses me.  It sounds like it means
"create a Location which can be used by element e", which doesn't really make
sense.

Similar comment applies to the "forMatch", "forNode", and "forUnit" factories
below.

Not a huge deal, though, so it's your call.

[scheglov, 2014/08/22 17:14:54]

You're right.
I will rename these factories/functions to "fromX".

+      _locationForElement(element);
 
   /**
-   * Create a Location based on an element and offset from the analyzer engine.
+   * Create a Location based on an [engine.SearchMatch].
    */
-  factory Location.fromOffset(engine.Element element, int offset, int length) =>
-      _locationFromOffset(element, offset, length);
+  factory Location.forMatch(engine.SearchMatch match) =>
+      _locationForMatch(match);
+
+  /**
+   * Create a Location based on an [engine.AstNode].
+   */
+  factory Location.forNode(engine.AstNode node) =>
+      _locationForNode(node);
+
+  /**
+   * Create a Location based on an [engine.CompilationUnit].
+   */
+  factory Location.forUnit(engine.CompilationUnit unit, engine.SourceRange range) =>
+      _locationForUnit(unit, range);
 
   Map<String, dynamic> toJson() {
     Map<String, dynamic> result = {};
     result["file"] = file;
     result["offset"] = offset;
     result["length"] = length;
     result["startLine"] = startLine;
     result["startColumn"] = startColumn;
     return result;
   }
@@ skipping 931 matching lines @@
     if (json is String) {
       try {
         return new RefactoringProblemSeverity(json);
       } catch(_) {
         // Fall through
       }
     }
     throw jsonDecoder.mismatch(jsonPath, "RefactoringProblemSeverity");
   }
 
+  /**
+   * Returns the [RefactoringProblemSeverity] with the maximal severity.
+   */
+  static RefactoringProblemSeverity max(RefactoringProblemSeverity a, RefactoringProblemSeverity b) =>

[Brian Wilkerson, 2014/08/22 16:32:17]

I guess I wasn't clear. I think this should be an instance method so that
instead of writing

  RefactoringProblemSeverity.max(first, second)

or

  maxRefactoringProblemSeverity(first, second)

we can write

  first.max(second)

The body of _maxRefactoringProblemSeverity could then be inlined.

(Similar comment applies for the constructors. What's the value of having a
factory constructor that just calls out to a top-level function?)

[scheglov, 2014/08/22 16:43:02]

  Sometimes both "first" and "second" are null.
  It works fine for RefactoringStatus.addError() for example, because we set
"second".
  But in RefactoringStatus.addStatus(other) both severity of the target
RefactoringStatus and "other" may be null.
  Of course we can check for null in clients, but why not make it an API?

  Alternatively we need the RefactoringProblemSeverity.OK value which is used
instead of null.
  I follow the existing style, which, as I understand, allows us to generate
minimal code using string literals and write actual code using IDE with all its
goodies like completion, refactorings, etc.

+      _maxRefactoringProblemSeverity(a, b);
+
   @override
   String toString() => "RefactoringProblemSeverity.$name";
 
   String toJson() => name;
 }
 
 /**
  * RemoveContentOverlay
  *
  * {
@@ skipping 1475 matching lines @@
     return false;
   }
 
   @override
   int get hashCode {
     int hash = 0;
     hash = _JenkinsSmiHash.combine(hash, newName.hashCode);
     return _JenkinsSmiHash.finish(hash);
   }
 }