added inherited methods and variables

added inherited methods and variables with docgen running in under 40s.


superclass ongoing.


attempting super class.


class map

BUG=
R=alanknight@google.com

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

[janicejl, 2013-07-29 17:22:58]

https://codereview.chromium.org/21096002/diff/5001/pkg/docgen/example/test.dart
File pkg/docgen/example/test.dart (left):

https://codereview.chromium.org/21096002/diff/5001/pkg/docgen/example/test.da...
pkg/docgen/example/test.dart:8: */
This file was deleted very early on (7-8 weeks ago), not sure why it is back.

[Alan Knight, 2013-07-29 20:29:00]

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

https://codereview.chromium.org/21096002/diff/10001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:60: /// Index of all the libraries that needs to be
outputted after all objects are
Nit. While contemporary dictionaries seem to consider "outputted" a word, I
think just "output" is better.

https://codereview.chromium.org/21096002/diff/10001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:378: if (clazz == null) {
This method is getting very long, and it looks like there's some commonality
that could be factored out, e.g. creating the class here, the superclass, and
the interfaces below below seem like something could be factored out into a
method that took just a mirror and maybe a bit more..

https://codereview.chromium.org/21096002/diff/10001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:406: if (key != 'constructors') {
This seems like it's crying out for an accessor like "instanceMethods" on Class,
or even "allNonConstructors", just to avoid this kind of manual filtering by
string.

https://codereview.chromium.org/21096002/diff/10001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:608: /// Inherited methods in the class.
I'm not clear why methods in the class itself seem to be indexed just by name,
but there's an additional level of categorization on inherited methods.

https://codereview.chromium.org/21096002/diff/10001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:610: 'setters': {},
It seems like it might be nicer if this an object, especially given the usage in
_getClasses. (Aside: I may have mentioned that I'm not fond of "get" in method
names :-)

https://codereview.chromium.org/21096002/diff/10001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:631: 'name': name,
I think literal maps can be indented just two.

[janicejl, 2013-07-30 00:41:31]

Based off your suggest, I did the same for the Map within a Map for classes as
well. This made outputting classes into separate files significantly easier. 

I also changed the unit test to make sure it still passes after adding
MethodGroup and ClassGroup.

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

https://codereview.chromium.org/21096002/diff/10001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:60: /// Index of all the libraries that needs to be
outputted after all objects are
On 2013/07/29 20:29:01, Alan Knight wrote:
> Nit. While contemporary dictionaries seem to consider "outputted" a word, I
> think just "output" is better.

Done.

https://codereview.chromium.org/21096002/diff/10001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:378: if (clazz == null) {
On 2013/07/29 20:29:01, Alan Knight wrote:
> This method is getting very long, and it looks like there's some commonality
> that could be factored out, e.g. creating the class here, the superclass, and
> the interfaces below below seem like something could be factored out into a
> method that took just a mirror and maybe a bit more..

Done.

https://codereview.chromium.org/21096002/diff/10001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:406: if (key != 'constructors') {
On 2013/07/29 20:29:01, Alan Knight wrote:
> This seems like it's crying out for an accessor like "instanceMethods" on
Class,
> or even "allNonConstructors", just to avoid this kind of manual filtering by
> string.

Done.

https://codereview.chromium.org/21096002/diff/10001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:610: 'setters': {},
On 2013/07/29 20:29:01, Alan Knight wrote:
> It seems like it might be nicer if this an object, especially given the usage
in
> _getClasses. (Aside: I may have mentioned that I'm not fond of "get" in method
> names :-)

Done.

https://codereview.chromium.org/21096002/diff/10001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:631: 'name': name,
On 2013/07/29 20:29:01, Alan Knight wrote:
> I think literal maps can be indented just two.

Done.

[Alan Knight, 2013-07-30 22:12:53]

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

https://codereview.chromium.org/21096002/diff/34001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:326: _currentMember = mirror;
This whole method  seems more like it should be a constructor or method for
adding on MethodGroup. Or maybe the iteration should be here but the inner
anonymous function should be a method on MethodGroup. We should generally let
the methodGroup manage its internal fields, not manipulate them from the
outside.

https://codereview.chromium.org/21096002/diff/34001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:349: Class _findOrCreateClass(ClassMirror mirror,
bool includePrivate) {
Naming nit. The fact that we're caching these isn't the fundamental part of the
API, so it doesn't really need to be in the name. This gives us back the class
for a mirror. So it could just be called _class, matching _variables, _methods,
etc.

[janicejl, 2013-08-01 04:30:26]

Makes a second pass to make sure that all the classes are valid, and none are
mixins (of the form: A extends B with C). 

I removed the qualifiedNameIndex as the way to organized the search index.txt,
and instead used the entity map to store everything indexable. 

All indexable objects now have an owner field which will store the qualified
name of the container that holds the object. This will allow me to easily modify
interconnected things during the second pass. (Like when changing a class to be
private, to make all it's method to be private as well. )

Sorry that this is such a huge CL, hope it would be relatively straightforward
to figure out what I was trying to do. I have added comments in as many places
as I can where I feel like there may be confusion (Or where I had to think hard
about it before I was sure that it is doing the right thing.)

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

https://codereview.chromium.org/21096002/diff/34001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:349: Class _findOrCreateClass(ClassMirror mirror,
bool includePrivate) {
On 2013/07/30 22:12:53, Alan Knight wrote:
> Naming nit. The fact that we're caching these isn't the fundamental part of
the
> API, so it doesn't really need to be in the name. This gives us back the class
> for a mirror. So it could just be called _class, matching _variables,
_methods,
> etc.

Done.

[Alan Knight, 2013-08-01 18:27:19]

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

https://codereview.chromium.org/21096002/diff/46001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:75: _includePrivate = includePrivate;
It's hard to follow this in the web view, but the comment for the variable says
this is set from the command-line. Wouldn't this overwrite it any time docgen is
called? Unless docgen is only called where it's passing in the command-line
argument value. In which case wouldn't it be easier to set the variable directly
from the command-line parsing logic rather than have it as a parameter here?

https://codereview.chromium.org/21096002/diff/46001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:213: // classes created from mixins are included.
I assume this means we're skipping intermediate entities created by the mixin
application, rather than excluding classes that happen to use mixins?

https://codereview.chromium.org/21096002/diff/46001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:221: _writeIndexableToFile(output, outputToYaml);
Nit.  I think you could re-arrange the filtering clauses and put some in
variables, and avoid a bit of duplication. e.g. filter the non-private, then
filtering classes or libraries based on that.

https://codereview.chromium.org/21096002/diff/46001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:256: * otherwise.
How does the "._" case arise? Should probably be documented. Or is it a bug?

https://codereview.chromium.org/21096002/diff/46001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:491: this.qualifiedName = qualifiedName;
Why are these two in the body of the constructor instead of just writing
"this.isPrivate"

https://codereview.chromium.org/21096002/diff/46001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:567: * methods as well as variables and methods from
the superclass.
These comments could be clearer.

https://codereview.chromium.org/21096002/diff/46001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:580: * If this class is private, it will add it to
it's superclasses.
Too many different "it"s. Refer to the things by name. And for "this class" you
can probably just say "[this]"

https://codereview.chromium.org/21096002/diff/46001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:582: void addSubClass(Class subclass) {
Be consistent between subClass and subclass. I think subclass is preferred, but
I'm not sure.

https://codereview.chromium.org/21096002/diff/46001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:594: * Ensures that the class is exists in the owner
library.
No "is", and probably better to say "Check" than "Ensures", as the latter
suggests doing something to make sure it's true. And if I'm understanding the
mixin logic, it might be better to say "mixin application" than just "mixin".

https://codereview.chromium.org/21096002/diff/46001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:655: clazz.interfaces.forEach((interface) {
The logic for superclasses and interfaces seems to be the same. Could you
combine them by iterating over
  var parent = clazz.superclass == null ? [] : [clazz.superclass];
  ([parent]..addAll(interfaces)).forEach(...

or something similar? And in fact you could write a method on Class returning
"parents" or some other suitable name, that returned a list with the superclass
and interfaces. Which would also generalize if we wanted to documented mixin
parents. And how does this work if we have a class mixins (real ones, not the
artificial ones that we skip)?

https://codereview.chromium.org/21096002/diff/46001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:691: .where((e) => (_includePrivate || !e.isPrivate))
This privacy test occurs a lot, seems like it could be a method/function.

https://codereview.chromium.org/21096002/diff/46001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:829: void addInherited(Class implemented) {
Why the name implemented here?

[janicejl, 2013-08-01 20:15:40]

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

https://codereview.chromium.org/21096002/diff/46001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:213: // classes created from mixins are included.
On 2013/08/01 18:27:19, Alan Knight wrote:
> I assume this means we're skipping intermediate entities created by the mixin
> application, rather than excluding classes that happen to use mixins?

Done.

https://codereview.chromium.org/21096002/diff/46001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:221: _writeIndexableToFile(output, outputToYaml);
On 2013/08/01 18:27:19, Alan Knight wrote:
> Nit.  I think you could re-arrange the filtering clauses and put some in
> variables, and avoid a bit of duplication. e.g. filter the non-private, then
> filtering classes or libraries based on that.

Done.

https://codereview.chromium.org/21096002/diff/46001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:256: * otherwise.
On 2013/08/01 18:27:19, Alan Knight wrote:
> How does the "._" case arise? Should probably be documented. Or is it a bug?

Done.

https://codereview.chromium.org/21096002/diff/46001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:491: this.qualifiedName = qualifiedName;
On 2013/08/01 18:27:19, Alan Knight wrote:
> Why are these two in the body of the constructor instead of just writing
> "this.isPrivate"

Done.

https://codereview.chromium.org/21096002/diff/46001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:567: * methods as well as variables and methods from
the superclass.
On 2013/08/01 18:27:19, Alan Knight wrote:
> These comments could be clearer. 

Done.

https://codereview.chromium.org/21096002/diff/46001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:580: * If this class is private, it will add it to
it's superclasses.
On 2013/08/01 18:27:19, Alan Knight wrote:
> Too many different "it"s. Refer to the things by name. And for "this class"
you
> can probably just say "[this]"

Done.

https://codereview.chromium.org/21096002/diff/46001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:582: void addSubClass(Class subclass) {
On 2013/08/01 18:27:19, Alan Knight wrote:
> Be consistent between subClass and subclass. I think subclass is preferred,
but
> I'm not sure.

Done.

https://codereview.chromium.org/21096002/diff/46001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:594: * Ensures that the class is exists in the owner
library.
On 2013/08/01 18:27:19, Alan Knight wrote:
> No "is", and probably better to say "Check" than "Ensures", as the latter
> suggests doing something to make sure it's true. And if I'm understanding the
> mixin logic, it might be better to say "mixin application" than just "mixin".

Done.

https://codereview.chromium.org/21096002/diff/46001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:655: clazz.interfaces.forEach((interface) {
On 2013/08/01 18:27:19, Alan Knight wrote:
> The logic for superclasses and interfaces seems to be the same. Could you
> combine them by iterating over
>   var parent = clazz.superclass == null ? [] : [clazz.superclass];
>   ([parent]..addAll(interfaces)).forEach(...
> 
> or something similar? And in fact you could write a method on Class returning
> "parents" or some other suitable name, that returned a list with the
superclass
> and interfaces. Which would also generalize if we wanted to documented mixin
> parents. And how does this work if we have a class mixins (real ones, not the
> artificial ones that we skip)?

Done. At this point, it does not differentiate between the artificial classes
and regular classes.

https://codereview.chromium.org/21096002/diff/46001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:691: .where((e) => (_includePrivate || !e.isPrivate))
On 2013/08/01 18:27:19, Alan Knight wrote:
> This privacy test occurs a lot, seems like it could be a method/function.

Done.

https://codereview.chromium.org/21096002/diff/46001/pkg/docgen/lib/docgen.dar...
pkg/docgen/lib/docgen.dart:829: void addInherited(Class implemented) {
On 2013/08/01 18:27:19, Alan Knight wrote:
> Why the name implemented here?

A parent class. I renamed implemented to parent.

[Alan Knight, 2013-08-01 20:36:48]

lgtm

[janicejl, 2013-08-01 20:51:08]

Committed patchset #21 manually as r25722 (presubmit successful).