Members and comments inherited in dartdoc.

Members and comments inherited in dartdoc.

BUG=1488, 3830, 1156, 5081
Committed: https://code.google.com/p/dart/source/detail?r=13225

[Johnni Winther, 2012-10-02 12:44:14]

On 2012/10/02 12:41:21, Johnni Winther wrote:

See the resulting API at
https://www.corp.google.com/~johnniwinther/no_crawl/api_docs

[Lasse Reichstein Nielsen, 2012-10-04 07:41:19]

LGTM with comments.

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/lib/dartdoc.dart
File pkg/dartdoc/lib/dartdoc.dart (right):

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/lib/dartdoc.da...
pkg/dartdoc/lib/dartdoc.dart:816: writeln('<span class="show-inherited">Hide
inherited</span>');
Use a button! This is clearly a clickable thing with a non-navigating behavior,
so it should be a button. 
Also, give it an id instead of a class. It should be unique in the document.

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/lib/dartdoc.da...
pkg/dartdoc/lib/dartdoc.dart:998: host.declaredMembers.forEach((_, MemberMirror
member) {
Add a host.forEachDeclaredMember method instead of acting directly on the map.
When you even ignore the key, it's obvious that the Map is an internal storage
representation that needn't be externally visible.

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/lib/dartdoc.da...
pkg/dartdoc/lib/dartdoc.dart:1041: writeln('<div>');
Why wrap it in a div if it doesn't even have a class?
Seems like pure overhead.

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/lib/dartdoc.da...
pkg/dartdoc/lib/dartdoc.dart:1538: match[2] == null ? typeName :
'$typeName.${match[2]}';
I recommend parentheses around any non-trivial condition in a condition
expression.

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/lib/dartdoc.da...
pkg/dartdoc/lib/dartdoc.dart:1641: String get html => md.markdownToHtml(text);
Why isn't the "inherited from" HTML added by this class, instead of doing it in
several places above?

If there are different versions of the comment, you could have different getters
for the different cases, but localized in the DocComment class.

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/lib/mirrors_ut...
File pkg/dartdoc/lib/mirrors_util.dart (right):

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/lib/mirrors_ut...
pkg/dartdoc/lib/mirrors_util.dart:76: class HierarchyIterable implements
Iterable<InterfaceMirror> {
Consider whether Iterable is necessary in the name. Maybe it is.

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/lib/mirrors_ut...
pkg/dartdoc/lib/mirrors_util.dart:92: * relation and finally is [Object]
visited.
Why do you mention Object explicitly? It is in the superinterfaces and
superclass relation of any class, even if it's implicitly, so it should be hit
by the earlier traversal.
Or is this only traversing the *declared* type relations (explicit
extends/implements classes)?
What is traversal order? Depth first or breadth first?(The queue suggests
breadth first).
If a type is implemented more than once, is it encountered more than once?
(Please document).

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/lib/src/client...
File pkg/dartdoc/lib/src/client/client-shared.dart (right):

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/lib/src/client...
pkg/dartdoc/lib/src/client/client-shared.dart:61: var showInherited =
document.query('.show-inherited');
Why is show-inherited a class and not an id? Do you ever need more than one?

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/lib/src/mirror...
File pkg/dartdoc/lib/src/mirrors/util.dart (right):

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/lib/src/mirror...
pkg/dartdoc/lib/src/mirrors/util.dart:149: if (value !== null) {
I think you can start using != instead of !==.

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/static/styles.css
File pkg/dartdoc/static/styles.css (right):

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/static/styles....
pkg/dartdoc/static/styles.css:18: /*margin: 0 0 22px 0;*/
Remove commented code.

[Johnni Winther, 2012-10-04 12:46:21]

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/lib/dartdoc.dart
File pkg/dartdoc/lib/dartdoc.dart (right):

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/lib/dartdoc.da...
pkg/dartdoc/lib/dartdoc.dart:816: writeln('<span class="show-inherited">Hide
inherited</span>');
On 2012/10/04 07:41:19, Lasse Reichstein Nielsen wrote:
> Use a button! This is clearly a clickable thing with a non-navigating
behavior,
> so it should be a button. 
> Also, give it an id instead of a class. It should be unique in the document.

Done.

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/lib/dartdoc.da...
pkg/dartdoc/lib/dartdoc.dart:998: host.declaredMembers.forEach((_, MemberMirror
member) {
On 2012/10/04 07:41:19, Lasse Reichstein Nielsen wrote:
> Add a host.forEachDeclaredMember method instead of acting directly on the map.
> When you even ignore the key, it's obvious that the Map is an internal storage
> representation that needn't be externally visible.

[declaredMembers] is also used for lookup where the key is used. This is
actually the specific case because we want to go through all members.

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/lib/dartdoc.da...
pkg/dartdoc/lib/dartdoc.dart:1041: writeln('<div>');
On 2012/10/04 07:41:19, Lasse Reichstein Nielsen wrote:
> Why wrap it in a div if it doesn't even have a class?
> Seems like pure overhead.

Not quite sure. It was requested in dartbug.com/1156 and has something to do
with user search.

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/lib/dartdoc.da...
pkg/dartdoc/lib/dartdoc.dart:1538: match[2] == null ? typeName :
'$typeName.${match[2]}';
On 2012/10/04 07:41:19, Lasse Reichstein Nielsen wrote:
> I recommend parentheses around any non-trivial condition in a condition
> expression.

Done.

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/lib/dartdoc.da...
pkg/dartdoc/lib/dartdoc.dart:1641: String get html => md.markdownToHtml(text);
On 2012/10/04 07:41:19, Lasse Reichstein Nielsen wrote:
> Why isn't the "inherited from" HTML added by this class, instead of doing it
in
> several places above?
> 
> If there are different versions of the comment, you could have different
getters
> for the different cases, but localized in the DocComment class.

The functionality might fit in the DocComment class if it had more information.
We want better diagnostics in parsing markdown, so it might all be put here in a
later CL.

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/lib/mirrors_ut...
File pkg/dartdoc/lib/mirrors_util.dart (right):

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/lib/mirrors_ut...
pkg/dartdoc/lib/mirrors_util.dart:76: class HierarchyIterable implements
Iterable<InterfaceMirror> {
On 2012/10/04 07:41:19, Lasse Reichstein Nielsen wrote:
> Consider whether Iterable is necessary in the name. Maybe it is.

I can't think of a sensible name without Iterable.

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/lib/mirrors_ut...
pkg/dartdoc/lib/mirrors_util.dart:92: * relation and finally is [Object]
visited.
On 2012/10/04 07:41:19, Lasse Reichstein Nielsen wrote:
> Why do you mention Object explicitly? It is in the superinterfaces and
> superclass relation of any class, even if it's implicitly, so it should be hit
> by the earlier traversal.
> Or is this only traversing the *declared* type relations (explicit
> extends/implements classes)?
> What is traversal order? Depth first or breadth first?(The queue suggests
> breadth first).
> If a type is implemented more than once, is it encountered more than once?
> (Please document).

Done.

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/lib/src/client...
File pkg/dartdoc/lib/src/client/client-shared.dart (right):

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/lib/src/client...
pkg/dartdoc/lib/src/client/client-shared.dart:61: var showInherited =
document.query('.show-inherited');
On 2012/10/04 07:41:19, Lasse Reichstein Nielsen wrote:
> Why is show-inherited a class and not an id? Do you ever need more than one?

Done.

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/lib/src/mirror...
File pkg/dartdoc/lib/src/mirrors/util.dart (right):

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/lib/src/mirror...
pkg/dartdoc/lib/src/mirrors/util.dart:149: if (value !== null) {
On 2012/10/04 07:41:19, Lasse Reichstein Nielsen wrote:
> I think you can start using != instead of !==.

Done.

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/static/styles.css
File pkg/dartdoc/static/styles.css (right):

https://codereview.chromium.org/10985085/diff/1001/pkg/dartdoc/static/styles....
pkg/dartdoc/static/styles.css:18: /*margin: 0 0 22px 0;*/
On 2012/10/04 07:41:19, Lasse Reichstein Nielsen wrote:
> Remove commented code.

Done.