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

pkg/analysis_server/lib/src/generated_protocol.dart

Index: pkg/analysis_server/lib/src/generated_protocol.dart
diff --git a/pkg/analysis_server/lib/src/generated_protocol.dart b/pkg/analysis_server/lib/src/generated_protocol.dart
index abf32c684e3bfbe0fcf8f5d16548eea8626d1627..4e7df5f1ed0c70d50f3bc88ffd476a19e41618c5 100644
--- a/pkg/analysis_server/lib/src/generated_protocol.dart
+++ b/pkg/analysis_server/lib/src/generated_protocol.dart
@@ -6622,16 +6622,28 @@ class Location implements HasToJson {
   }
 
   /**
-   * Construct based on an element from the analyzer engine.
+   * Create a Location based on an [engine.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 [engine.SearchMatch].
+   */
+  factory Location.forMatch(engine.SearchMatch match) =>
+      _locationForMatch(match);
+
+  /**
+   * Create a Location based on an [engine.AstNode].
    */
-  factory Location.fromElement(engine.Element element) =>
-      _locationFromElement(element);
+  factory Location.forNode(engine.AstNode node) =>
+      _locationForNode(node);
 
   /**
-   * Create a Location based on an element and offset from the analyzer engine.
+   * Create a Location based on an [engine.CompilationUnit].
    */
-  factory Location.fromOffset(engine.Element element, int offset, int length) =>
-      _locationFromOffset(element, offset, length);
+  factory Location.forUnit(engine.CompilationUnit unit, engine.SourceRange range) =>
+      _locationForUnit(unit, range);
 
   Map<String, dynamic> toJson() {
     Map<String, dynamic> result = {};
@@ -7583,6 +7595,12 @@ class RefactoringProblemSeverity {
     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";