Added inherited comments, and where they are inherited from.

Added inherited comments, and where they are inherited from.

R=efortuna@google.com

Committed: https://code.google.com/p/dart/source/detail?r=25748

[Alan Knight, 2013-08-02 18:32:22]

https://codereview.chromium.org/21619003/diff/1/pkg/docgen/lib/docgen.dart
File pkg/docgen/lib/docgen.dart (right):

https://codereview.chromium.org/21619003/diff/1/pkg/docgen/lib/docgen.dart#ne...
pkg/docgen/lib/docgen.dart:628: var method = methods[qualifiedName];
Since you're actually using the value, this might be more easily written as
   inheritedMethods.forEach((qualifiedName, inheritedMethod) {
...

https://codereview.chromium.org/21619003/diff/1/pkg/docgen/lib/docgen.dart#ne...
pkg/docgen/lib/docgen.dart:630: if (method.comment == "") {
I think this might also work better if you named the inner method and used a
guard clause rather than nesting. e.g.

ensureCommentFor(qualifiedName, inheritedMethod) {
  var method = methods[qualifiedName];
  if (method == null) return;

https://codereview.chromium.org/21619003/diff/1/pkg/docgen/lib/docgen.dart#ne...
pkg/docgen/lib/docgen.dart:633: if (method.comment != "") {
In fact, I think this could be rewritten to be much simpler. Is it true that we
never want to overwrite a non-empty comment/commentInheritedFrom? If so, why not
put that logic into the Method class so we can just write
   method.commentInheritedFrom = inheritedMethod.commentInheritedFrom;
And similarly for the comment. If it's not always true, then we could provide a
variant setter that doesn't overwrite.

Then the whole thing can become something like
  Method.ensureCommentFor(inheritedMethod) {
     if (comment.isNotEmpty) return;
     entityMap[inheritedMethod.owner].ensureComments();
     method.comment = inheritedMethod.comment;
     method.commentInheritedFrom = inheritedMethod.commentInheritedFrom;
 }

That might be slightly inefficient, as I think it will check the whole of the
superclass for each method that doesn't have a comment in the subclass, but
there's probably a way to find just the inherited method and ensureComments on
it individually.

https://codereview.chromium.org/21619003/diff/1/pkg/docgen/lib/docgen.dart#ne...
pkg/docgen/lib/docgen.dart:883: var method = setters[qualifiedName];
I don't love the deep nesting. Redo as a loop over [setters, getters, ...]?

https://codereview.chromium.org/21619003/diff/1/pkg/docgen/lib/docgen.dart#ne...
pkg/docgen/lib/docgen.dart:900: Iterable<String> get keys {
=> [setters, getters, operators, regularMethods].expand((x) => x.keys);

with optional toList() on the end.

[janicejl, 2013-08-02 21:40:27]

https://codereview.chromium.org/21619003/diff/1/pkg/docgen/lib/docgen.dart
File pkg/docgen/lib/docgen.dart (right):

https://codereview.chromium.org/21619003/diff/1/pkg/docgen/lib/docgen.dart#ne...
pkg/docgen/lib/docgen.dart:628: var method = methods[qualifiedName];
On 2013/08/02 18:32:22, Alan Knight wrote:
> Since you're actually using the value, this might be more easily written as
>    inheritedMethods.forEach((qualifiedName, inheritedMethod) {
> ...
> 

Done.

https://codereview.chromium.org/21619003/diff/1/pkg/docgen/lib/docgen.dart#ne...
pkg/docgen/lib/docgen.dart:630: if (method.comment == "") {
On 2013/08/02 18:32:22, Alan Knight wrote:
> I think this might also work better if you named the inner method and used a
> guard clause rather than nesting. e.g.
> 
> ensureCommentFor(qualifiedName, inheritedMethod) {
>   var method = methods[qualifiedName];
>   if (method == null) return;
>   

Done.

https://codereview.chromium.org/21619003/diff/1/pkg/docgen/lib/docgen.dart#ne...
pkg/docgen/lib/docgen.dart:633: if (method.comment != "") {
On 2013/08/02 18:32:22, Alan Knight wrote:
> In fact, I think this could be rewritten to be much simpler. Is it true that
we
> never want to overwrite a non-empty comment/commentInheritedFrom? If so, why
not
> put that logic into the Method class so we can just write
>    method.commentInheritedFrom = inheritedMethod.commentInheritedFrom;
> And similarly for the comment. If it's not always true, then we could provide
a
> variant setter that doesn't overwrite.
> 
> Then the whole thing can become something like
>   Method.ensureCommentFor(inheritedMethod) {
>      if (comment.isNotEmpty) return;
>      entityMap[inheritedMethod.owner].ensureComments();
>      method.comment = inheritedMethod.comment;
>      method.commentInheritedFrom = inheritedMethod.commentInheritedFrom;
>  }
> 
> That might be slightly inefficient, as I think it will check the whole of the
> superclass for each method that doesn't have a comment in the subclass, but
> there's probably a way to find just the inherited method and ensureComments on
> it individually.

Done.

https://codereview.chromium.org/21619003/diff/1/pkg/docgen/lib/docgen.dart#ne...
pkg/docgen/lib/docgen.dart:883: var method = setters[qualifiedName];
On 2013/08/02 18:32:22, Alan Knight wrote:
> I don't love the deep nesting. Redo as a loop over [setters, getters, ...]?

Done.

https://codereview.chromium.org/21619003/diff/1/pkg/docgen/lib/docgen.dart#ne...
pkg/docgen/lib/docgen.dart:900: Iterable<String> get keys {
On 2013/08/02 18:32:22, Alan Knight wrote:
> => [setters, getters, operators, regularMethods].expand((x) => x.keys);
> 
> with optional toList() on the end.

Done.

[Emily Fortuna, 2013-08-02 21:50:47]

lgtm

[janicejl, 2013-08-02 22:19:28]

Committed patchset #2 manually as r25748 (presubmit successful).