Fields, getters and setters documented as properties.

pkg/dartdoc/lib/dartdoc.dart

Index: pkg/dartdoc/lib/dartdoc.dart
diff --git a/pkg/dartdoc/lib/dartdoc.dart b/pkg/dartdoc/lib/dartdoc.dart
index 8e9a159b64803886506961f81353e9eb8a0b9e7e..540cb139d62f1368c3ff37983ac3d50d1055eb1d 100644
--- a/pkg/dartdoc/lib/dartdoc.dart
+++ b/pkg/dartdoc/lib/dartdoc.dart
@@ -819,7 +819,11 @@ class Dartdoc {
     writeln('<button id="show-inherited" class="show-inherited">'
             'Hide inherited</button>');
 
-    docCode(type, type.location, getTypeComment(type));
+    writeln('<div class="doc">');
+    docComment(type, getTypeComment(type));
+    docCode(type, type.location);
+    writeln('</div>');
+
     docInheritance(type);
     docTypedef(type);
 
@@ -964,7 +968,9 @@ class Dartdoc {
               title="Permalink to ${type.simpleName}">#</a>''');
     writeln('</h4>');
 
-    docCode(type, type.location, null, showCode: true);
+    writeln('<div class="doc">');
+    docCode(type, type.location);
+    writeln('</div>');
 
     writeln('</div>');
   }
@@ -991,21 +997,39 @@ class Dartdoc {
     }
   }
 
+  const operatorOrder = const <String>[
+      '[]', '[]=', // Indexing.
+      '+', Mirror.UNARY_MINUS, '-', '*', '/', '~/', '%', // Arithmetic.
+      '&', '|', '^', '~', // Bitwise.
+      '<<', '>>', // Shift.
+      '<', '<=', '>', '>=', // Relational.
+      '==', // Equality.
+  ];
+
   void docMembers(ObjectMirror host) {
     // Collect the different kinds of members.
     final staticMethods = [];
-    final staticFields = [];
+    final staticGetters = new Map<String,MemberMirror>();

[Bob Nystrom, 2012/10/11 17:04:21]

Style nit: there should be spaces after the commas here.

[Lasse Reichstein Nielsen, 2012/10/12 09:47:26]

I'm getting less and less attached to that space (and I used to complain about
it missing too). 
It really breaks up the type and makes it harder to scan pat them to get from
the "new ClassName" to the parameters.
I.e., I see, and read, "new Foo<Bar,Baz>()" by seeing that it's a
"new"-expression, it's to the type Foo with some type parameters, and then I
skip to read the parameters. Only if I have reason to check will I actually read
the type parameters. Scanning "new Foo<Bar, Baz>()" is harder because I stop at
the space and need to mentally recognize that I need to continue.

[Johnni Winther, 2012/10/12 11:15:53]

I would even vote for a style guide change. Consider for example the signature
in lines 1091-1093. Putting spaces in the type annotation would make it look
there are more parameters than is the case.

[Bob Nystrom, 2012/10/12 16:09:02]

I personally don't have any trouble reading it with spaces after the ",". Java's
style of not using "," only after type parameters always seemed arbitrary to me
but I don't have a strong preference one way or the other.

At least for now, the style guideline is that there should be a space after it.
If you'd like, we discuss revising it to but that should be a dart-team@
discussion and not just here.

+    final staticSetters = new Map<String,MemberMirror>();
     final memberMap = new Map<String,MemberMirror>();
     final instanceMethods = [];
-    final instanceFields = [];
+    final instanceOperators = [];
+    final instanceGetters = new Map<String,MemberMirror>();
+    final instanceSetters = new Map<String,MemberMirror>();
 
     host.declaredMembers.forEach((_, MemberMirror member) {
       if (member.isPrivate) return;
       if (host is LibraryMirror || member.isStatic) {
-        if (member.isMethod) {
-          staticMethods.add(member);
-        } else  if (member.isField) {
-          staticFields.add(member);
+        if (member is MethodMirror) {
+                    if (member.isGetter) {

[Bob Nystrom, 2012/10/11 17:04:21]

What happened to the indentation here?

[Johnni Winther, 2012/10/12 11:15:53]

Done.

+            staticGetters[member.displayName] = member;
+          } else if (member.isSetter) {
+            staticSetters[member.displayName] = member;
+          } else {
+            staticMethods.add(member);
+          }
+        } else if (member is FieldMirror) {
+          staticGetters[member.displayName] = member;
         }
       }
     });
@@ -1016,53 +1040,117 @@ class Dartdoc {
         type.declaredMembers.forEach((_, MemberMirror member) {
           if (member.isPrivate) return;
           if (!member.isStatic) {
-            memberMap.putIfAbsent(member.simpleName, () => member);
+            if (member.isField) {
+              // Fields override both getters and setters.
+              memberMap.putIfAbsent(member.simpleName, () => member);
+              memberMap.putIfAbsent('${member.simpleName}=', () => member);
+            } else {
+              memberMap.putIfAbsent(member.simpleName, () => member);
+            }
           }
         });
       }
     }
 
     memberMap.forEach((_, MemberMirror member) {
-      if (member.isMethod) {
-        instanceMethods.add(member);
-      } else  if (member.isField) {
-        instanceFields.add(member);
+      if (member is MethodMirror) {
+        if (member.isGetter) {
+          instanceGetters[member.displayName] = member;
+        } else if (member.isSetter) {
+          instanceSetters[member.displayName] = member;
+        } else if (member.isOperator) {
+          instanceOperators.add(member);
+        } else {
+          instanceMethods.add(member);
+        }
+      } else if (member is FieldMirror) {
+        instanceGetters[member.displayName] = member;
       }
     });
 
-    if (staticFields.length > 0) {
-      final title = host is LibraryMirror ? 'Variables' : 'Static Fields';
-      writeln('<div>');
-      writeln('<h3>$title</h3>');
-      for (final field in orderByName(staticFields)) {
-        docField(host, field);
-      }
-      writeln('</div>');
-    }
+    instanceOperators.sort((MethodMirror a, MethodMirror b) {
+      return operatorOrder.indexOf(a.simpleName).compareTo(
+          operatorOrder.indexOf(b.simpleName));

[Lasse Reichstein Nielsen, 2012/10/12 09:47:26]

How inefficient. How about making the operatorOrder a Map<String,int> that
returns the priority directly, instead of doing a linear lookup in a list twice
per comparison in the sort?

[Johnni Winther, 2012/10/12 11:15:53]

Done.

+    });
 
-    if (staticMethods.length > 0) {
-      final title = host is LibraryMirror ? 'Functions' : 'Static Methods';
-      writeln('<div>');
-      writeln('<h3>$title</h3>');
-      for (final method in orderByName(staticMethods)) {
-        docMethod(host, method);
-      }
-      writeln('</div>');
-    }
+    docProperties(host,
+                  host is LibraryMirror ? 'Properties' : 'Static Properties',
+                  staticGetters, staticSetters);
+    docMethods(host,
+               host is LibraryMirror ? 'Functions' : 'Static Methods',
+               staticMethods);
+
+    docProperties(host, 'Properties', instanceGetters, instanceSetters);
+    docMethods(host, 'Operators', instanceOperators);
+    docMethods(host, 'Methods', orderByName(instanceMethods));
+  }
+
+  /**
+   * Documents fields, getters, and setters as properties.
+   */
+  void docProperties(ObjectMirror host, String title,
+                     Map<String,MemberMirror> getters,
+                     Map<String,MemberMirror> setters) {
+    if (getters.length + setters.length > 0) {

[Bob Nystrom, 2012/10/11 17:04:21]

Do an early return here and lose a level of indentation:

if (getters.isEmpty() && setters.isEmpty()) return;

[Lasse Reichstein Nielsen, 2012/10/12 09:47:26]

Agree violently.

[Johnni Winther, 2012/10/12 11:15:53]

Done.

+      var nameSet = new Set<String>.from(getters.getKeys());
+      nameSet.addAll(setters.getKeys());
+      var nameList = new List<String>.from(nameSet);
+      nameList.sort((String a, String b) {
+        return a.toLowerCase().compareTo(b.toLowerCase());

[Lasse Reichstein Nielsen, 2012/10/12 09:47:26]

Seems wasteful to do toLowerCase twice on every call (but I know you've asked
for compareToIgnoreCase :).

+      });
 
-    if (instanceFields.length > 0) {
       writeln('<div>');
-      writeln('<h3>Fields</h3>');
-      for (final field in orderByName(instanceFields)) {
-        docField(host, field);
+      writeln('<h3>$title</h3>');
+      for (String name in nameList) {
+        MemberMirror getter = getters[name];
+        MemberMirror setter = setters[name];

[Bob Nystrom, 2012/10/11 17:04:21]

You can just use var here and elsewhere. Type annotations on locals make Dart
look painfully verbose.

[Lasse Reichstein Nielsen, 2012/10/12 09:47:26]

I disagree. I can't read the type of 'getter' from the right-hand-side, and
really appreciate (and demand!) the type be there to inform me.
Verbose, yes, but the pain to me is when it's not there, and I have to think
every time I read the line.

[Bob Nystrom, 2012/10/12 16:09:02]

That's the tool's job! :) Programmers shouldn't have to manually add redundant
information to their code that can be automated.

One thing I'm very sensitive to is the aesthetics of Dart code. People will
judge the language by how a page of code in it looks. To anyone coming from
JavaScript, Python, C#, or pretty much any language except Java, type
annotations on locals are a very strong turn-off.

I don't think the (negligible) additional information that local type annotation
provides is worth alienating potential users.

[Lasse Reichstein Nielsen, 2012/10/15 09:16:20]

I'm very much in the "must be readable when printed" faction here. You can't
control which tools are used to view or edit the code, so it should be readable
in all of them.
By all means use a pre-processor to add the types before committing :)
 
That might be my problem - I come from both (enterprise) Java and JavaScript,
and I find Java easier to read. Not easier to write, but that wasn't the design
intent of Java - it is designed to always be readable by making nothing
implicit. 
 
It's not negligible to me - I can't read and understand the code without that
information. If I had written the code originally, or edited it, or if I had had
to read the entire file (or library) first, I would probably understand it, but
as an outsider, the code is simply not understandable without more information. 
You can put the extra information in the variable name (setterMirror), or you
can add it as a type assertion (MemberMirror setter, giving you a shorter name
elsewhere), I won't care, but omitting it isn't good for readability, and
therefore not good for maintainability.
If the code isn't just a quick hack (which it is great that we also support),
maintainability is more important than ... pretty much everything except
correctness.

+        if (setter == null) {
+          if (getter is FieldMirror) {
+            // We have a field.
+            docField(host, getter);
+          } else {
+            // We only have a getter.
+            assert(getter is MethodMirror);
+            docProperty(host, getter, null);
+          }
+        } else if (getter == null) {
+          // We only have a setter => Document as a method.
+          assert(setter is MethodMirror);
+          docMethod(host, setter);
+        } else {
+          DocComment getterComment = getMemberComment(getter);
+          DocComment setterComment = getMemberComment(setter);
+          if (getter.surroundingDeclaration !== setter.surroundingDeclaration ||
+              getterComment != null && setterComment != null) {
+            // Both have comments or are not declared in the same class
+            // => Documents separately.
+            if (getter is FieldMirror) {
+              // Document field as a getter (setter is inherited).
+              docField(host, getter, asGetter: true);
+            } else {
+              docMethod(host, getter);
+            }
+            if (setter is FieldMirror) {
+              // Document field as a setter (getter is inherited).
+              docField(host, setter, asSetter: true);
+            } else {
+              docMethod(host, setter);
+            }
+          } else {
+            // Document as field.
+            docProperty(host, getter, setter);
+          }
+        }
       }
       writeln('</div>');
     }
+  }
 
-    if (instanceMethods.length > 0) {
+  void docMethods(ObjectMirror host, String title, List<MethodMirror> methods) {
+    if (methods.length > 0) {
       writeln('<div>');
-      writeln('<h3>Methods</h3>');
-      for (final method in orderByName(instanceMethods)) {
+      writeln('<h3>$title</h3>');
+      for (final method in methods) {
         docMethod(host, method);
       }
       writeln('</div>');
@@ -1070,104 +1158,189 @@ class Dartdoc {
   }
 
   /**
-   * Documents the [method] in type [type]. Handles all kinds of methods
-   * including getters, setters, and constructors.
+   * Documents the [member] in declared in [host]. Handles all kinds of methods

[Bob Nystrom, 2012/10/11 17:04:21]

"in declared" -> "declared"
"methods" -> "members"

[Johnni Winther, 2012/10/12 11:15:53]

Done.

+   * including getters, setters, and constructors. If [member] is a
+   * [FieldMirror] it is documented as a getter or setter depending upon the
+   * value of [asGetter].
    */
-  void docMethod(ObjectMirror host, MethodMirror method) {
+  void docMethod(ObjectMirror host, MemberMirror member,
+                 {bool asGetter: false}) {
     _totalMembers++;
-    _currentMember = method;
+    _currentMember = member;
+
+    bool isAbstract = false;
+    String name = member.displayName;
+    if (member is FieldMirror) {
+      if (asGetter) {
+        // Getter.
+        name = 'get $name';
+      } else {
+        // Setter.
+        name = 'set $name';
+      }
+    } else {
+      assert(member is MethodMirror);
+      isAbstract = member.isAbstract;
+      if (member.isGetter) {
+        // Getter.
+        name = 'get $name';
+      } else if (member.isSetter) {
+        // Setter.
+        name = 'set $name';
+      }
+    }
 
-    bool showCode = includeSource && !method.isAbstract;
-    bool inherited = host != method.surroundingDeclaration;
+    bool showCode = includeSource && !isAbstract;
+    bool inherited = host != member.surroundingDeclaration;
 
     writeln('<div class="method${inherited ? ' inherited': ''}">'
-            '<h4 id="${memberAnchor(method)}">');
+            '<h4 id="${memberAnchor(member)}">');
 
     if (showCode) {
       writeln('<button class="show-code">Code</button>');
     }
 
-    if (method.isConstructor) {
-      if (method.isFactory) {
-        write('factory ');
-      } else {
-        write(method.isConst ? 'const ' : 'new ');
+    if (member is MethodMirror) {
+      if (member.isConstructor) {
+        if (member.isFactory) {
+          write('factory ');
+        } else {
+          write(member.isConst ? 'const ' : 'new ');
+        }
+      } else if (member.isAbstract) {
+        write('abstract ');
       }
-    } else if (method.isAbstract) {
-      write('abstract ');
-    }
-
-    if (!method.isConstructor) {
-      annotateType(host, method.returnType);
-    }
 
-    var name = method.displayName;
-    // Translate specially-named methods: getters, setters, operators.
-    if (method.isGetter) {
-      // Getter.
-      name = 'get $name';
-    } else if (method.isSetter) {
-      // Setter.
-      name = 'set $name';
+      if (!member.isConstructor) {
+        annotateType(host, member.returnType);
+      }
+    } else {
+      assert(member is FieldMirror);
+      if (asGetter) {
+        annotateType(host, member.type);
+      } else {
+        write('void ');
+      }
     }
 
     write('<strong>$name</strong>');
 
-    docParamList(host, method.parameters);
+    if (member is MethodMirror) {
+      if (!member.isGetter) {
+        docParamList(host, member.parameters);
+      }
+    } else {
+      assert(member is FieldMirror);
+      if (!asGetter) {
+        write('(');
+        annotateType(host, member.type);
+        write(' _)');

[Bob Nystrom, 2012/10/11 17:04:21]

"_" -> "value"?

[Lasse Reichstein Nielsen, 2012/10/12 09:47:26]

Let's do that. C# programmers will be happy.

[Johnni Winther, 2012/10/12 11:15:53]

Done.

[Bob Nystrom, 2012/10/12 16:09:02]

Yay!

+      }
+    }
 
     var prefix = host is LibraryMirror ? '' : '${typeName(host)}.';
-    write(''' <a class="anchor-link" href="#${memberAnchor(method)}"
+    write(''' <a class="anchor-link" href="#${memberAnchor(member)}"
               title="Permalink to $prefix$name">#</a>''');
     writeln('</h4>');
 
     if (inherited) {
       write('<div class="inherited-from">inherited from ');
-      annotateType(host, method.surroundingDeclaration);
+      annotateType(host, member.surroundingDeclaration);
       write('</div>');
     }
 
-    docCode(host, method.location, getMemberComment(method), showCode: showCode);
+    writeln('<div class="doc">');
+    docComment(host, getMemberComment(member));
+    if (showCode) {
+      docCode(host, member.location);
+    }
+    writeln('</div>');
 
     writeln('</div>');
   }
 
-  /** Documents the field [field] of type [type]. */
-  void docField(ObjectMirror host, FieldMirror field) {
+  void docField(ObjectMirror host, FieldMirror field,
+                {bool asGetter: false, bool asSetter: false}) {
+    if (asGetter) {
+      docMethod(host, field, asGetter: true);
+    } else if (asSetter) {
+      docMethod(host, field, asGetter: false);
+    } else {
+      docProperty(host, field, null);
+    }
+  }
+
+  /**
+   * Documents the property defined by [property] of [type].
+   *
+   * If [comment] is not set, the comment on [property] is used for
+   * documentation. [additionalCodeLocation] is set to generate an additional
+   * code fragment is case of properties defined in terms of getters and
+   * setters.

[Bob Nystrom, 2012/10/11 17:04:21]

This comment is out of sync with the method.

[Johnni Winther, 2012/10/12 11:15:53]

Done.

+   */
+  void docProperty(ObjectMirror host,
+                   MemberMirror getter, MemberMirror setter) {
+    assert(getter != null);

[Bob Nystrom, 2012/10/11 17:04:21]

We generally assume all parameters are non-null and don't bother asserting or
validating that. Why check getter here?

[Johnni Winther, 2012/10/12 11:15:53]

[setter] might be null, but [getter] may not.

     _totalMembers++;
-    _currentMember = field;
+    _currentMember = getter;
 
-    bool inherited = host != field.surroundingDeclaration;
+    bool inherited = host != getter.surroundingDeclaration;
 
     writeln('<div class="field${inherited ? ' inherited' : ''}">'
-            '<h4 id="${memberAnchor(field)}">');
+            '<h4 id="${memberAnchor(getter)}">');
 
     if (includeSource) {
       writeln('<button class="show-code">Code</button>');
     }
 
-    if (field.isFinal) {
+    bool isConst = false;
+    bool isFinal;
+    TypeMirror type;
+    if (getter is FieldMirror) {
+      isConst = getter.isConst;
+      isFinal = getter.isFinal;
+      type = getter.type;
+    } else {
+      assert(getter is MethodMirror);
+      isFinal = setter == null;
+      type = getter.returnType;
+    }
+
+    if (isConst) {
+      write('const ');
+    } else if (isFinal) {
       write('final ');
-    } else if (field.type.isDynamic) {
+    } else if (type.isDynamic) {
       write('var ');
     }
 
-    annotateType(host, field.type);
+    annotateType(host, type);
     var prefix = host is LibraryMirror ? '' : '${typeName(host)}.';
     write(
         '''
-        <strong>${field.simpleName}</strong> <a class="anchor-link"
-            href="#${memberAnchor(field)}"
-            title="Permalink to $prefix${field.simpleName}">#</a>
+        <strong>${getter.simpleName}</strong> <a class="anchor-link"
+            href="#${memberAnchor(getter)}"
+            title="Permalink to $prefix${getter.simpleName}">#</a>
         </h4>
         ''');
 
     if (inherited) {
       write('<div class="inherited-from">inherited from ');
-      annotateType(host, field.surroundingDeclaration);
+      annotateType(host, getter.surroundingDeclaration);
       write('</div>');
     }
 
-    docCode(host, field.location, getMemberComment(field), showCode: true);
+    DocComment comment = getMemberComment(getter);
+    if (comment == null && setter != null) {
+      comment = getMemberComment(setter);
+    }
+    writeln('<div class="doc">');
+    docComment(host, comment);
+    docCode(host, getter.location);
+    if (setter != null) {
+      docCode(host, setter.location);
+    }
+    writeln('</div>');
 
     writeln('</div>');
   }
@@ -1200,13 +1373,7 @@ class Dartdoc {
     write(')');
   }
 
-  /**
-   * Documents the code contained within [span] with [comment]. If [showCode]
-   * is `true` (and [includeSource] is set), also includes the source code.
-   */
-  void docCode(ObjectMirror host, Location location, DocComment comment,
-               [bool showCode = false]) {
-    writeln('<div class="doc">');
+  void docComment(ObjectMirror host, DocComment comment) {
     if (comment != null) {
       if (comment.inheritedFrom !== null) {
         writeln('<div class="inherited">');
@@ -1219,14 +1386,18 @@ class Dartdoc {
         writeln(comment.html);
       }
     }
+  }
 
-    if (includeSource && showCode) {
+  /**
+   * Documents the code contained within [span] with [comment]. If [showCode]
+   * is `true` (and [includeSource] is set), also includes the source code.
+   */

[Bob Nystrom, 2012/10/11 17:04:21]

This comment is out of sync with the method.

[Johnni Winther, 2012/10/12 11:15:53]

Done.

+  void docCode(ObjectMirror host, Location location) {
+    if (includeSource) {
       writeln('<pre class="source">');
       writeln(md.escapeHtml(unindentCode(location)));
       writeln('</pre>');
     }
-
-    writeln('</div>');
   }
 
   DocComment createDocComment(String text, [InterfaceMirror inheritedFrom]) =>