pkg/docgen: refactoring and cleanup

pkg/docgen: refactoring and cleanup

locking down Indexable
Moved MDN code into its own library

R=efortuna@google.com

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

[kevmoo, 2014-03-06 18:45:43]

I started looking at a few fixes, but understanding the code was killing me.

Doing a clean-up pass first.

I verified that the SDK output is the same.

[Alan Knight, 2014-03-06 18:55:44]

Is it possible to split out "library move-around" from other changes. It's very
hard with Rietveld to review "this entire file deleted", "something that might
be the same or might have changes added somewhere else"

[kevmoo, 2014-03-06 19:18:09]

On 2014/03/06 18:55:44, Alan Knight wrote:
> Is it possible to split out "library move-around" from other changes. It's
very
> hard with Rietveld to review "this entire file deleted", "something that might
> be the same or might have changes added somewhere else"

PTAL

[Emily Fortuna, 2014-03-06 22:05:38]

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

https://codereview.chromium.org/180243024/diff/20001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:827: /** Walk up the owner chain to find the owning
library. */
Consistency! Triple slashes are everywhere through this file!

https://codereview.chromium.org/180243024/diff/20001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:828: Library _getOwningLibrary(Indexable indexable) {
make static function instead of global?

https://codereview.chromium.org/180243024/diff/20001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:1797: annotations = _createAnnotations(mirror,
owningLibrary);
is having a bunch of random global functions really better? I'm not certain.

https://codereview.chromium.org/180243024/diff/20001/pkg/docgen/lib/src/model...
File pkg/docgen/lib/src/models.dart (right):

https://codereview.chromium.org/180243024/diff/20001/pkg/docgen/lib/src/model...
pkg/docgen/lib/src/models.dart:24: }
what about putting Type and Parameter here, too? Those also only extend
MirrorBased.

[kevmoo, 2014-03-06 22:20:17]

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

https://codereview.chromium.org/180243024/diff/20001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:827: /** Walk up the owner chain to find the owning
library. */
On 2014/03/06 22:05:38, Emily Fortuna wrote:
> Consistency! Triple slashes are everywhere through this file!

Sticking with copy-paste the old style for this CL. Another CL to cleanup up
comments.

https://codereview.chromium.org/180243024/diff/20001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:828: Library _getOwningLibrary(Indexable indexable) {
On 2014/03/06 22:05:38, Emily Fortuna wrote:
> make static function instead of global?

If the static uses privates on the class or make usage more clear, sure.

Otherwise, having thing top-level make cross-class usage easier (don't need to
qualify) and it makes dependencies clear (_getOwningLibrary doesn't need
anything special from Indexable)

https://codereview.chromium.org/180243024/diff/20001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:1797: annotations = _createAnnotations(mirror,
owningLibrary);
On 2014/03/06 22:05:38, Emily Fortuna wrote:
> is having a bunch of random global functions really better? I'm not certain.

Random top-level methods make dependencies clear. Not using something special or
private to a given class IMHO

https://codereview.chromium.org/180243024/diff/20001/pkg/docgen/lib/src/model...
File pkg/docgen/lib/src/models.dart (right):

https://codereview.chromium.org/180243024/diff/20001/pkg/docgen/lib/src/model...
pkg/docgen/lib/src/models.dart:24: }
On 2014/03/06 22:05:38, Emily Fortuna wrote:
> what about putting Type and Parameter here, too? Those also only extend
> MirrorBased.

I wish, but there is a lot of dependency spaghetti. This is a first stab.

[kevmoo, 2014-03-11 22:07:31]

PTAL?

[Alan Knight, 2014-03-12 18:17:56]

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

https://codereview.chromium.org/180243024/diff/20001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:828: Library _getOwningLibrary(Indexable indexable) {
On 2014/03/06 22:20:17, kevmoo wrote:
> On 2014/03/06 22:05:38, Emily Fortuna wrote:
> > make static function instead of global?
> 
> If the static uses privates on the class or make usage more clear, sure.
> 
> Otherwise, having thing top-level make cross-class usage easier (don't need to
> qualify) and it makes dependencies clear (_getOwningLibrary doesn't need
> anything special from Indexable)

Why static or global at all? Why not just have this as a getter or zero-arg
method on Indexable and Library?

Library get owningLibrary =>  owner.owningLibrary

Library get owningLibrary => this;

Also Home is an Indexable that won't have an owningLibrary and may have a null
owner. So you either need a null check or an implementation in Home as well.

https://codereview.chromium.org/180243024/diff/20001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:852: final DeclarationMirror mirror;
If we're re-formatting these to have their own lines, shouldn't they also have
comments?

https://codereview.chromium.org/180243024/diff/20001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:1797: annotations = _createAnnotations(mirror,
owningLibrary);
On 2014/03/06 22:20:17, kevmoo wrote:
> On 2014/03/06 22:05:38, Emily Fortuna wrote:
> > is having a bunch of random global functions really better? I'm not certain.
> 
> Random top-level methods make dependencies clear. Not using something special
or
> private to a given class IMHO

I don't think it's about strict dependency requirements. This is only ever used
in the construction of a MirrorBased. Having it as a top-level function makes it
top-level noise when it's really an internal detail of MirrorBased. It would be
better as an instance method but I presume it can't be because it's called in
the constructor.

https://codereview.chromium.org/180243024/diff/20001/pkg/docgen/lib/src/model...
File pkg/docgen/lib/src/models.dart (right):

https://codereview.chromium.org/180243024/diff/20001/pkg/docgen/lib/src/model...
pkg/docgen/lib/src/models.dart:1: library docgen.moedls;
typo

[kevmoo, 2014-03-17 02:01:29]

PTAL

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

https://codereview.chromium.org/180243024/diff/20001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:828: Library _getOwningLibrary(Indexable indexable) {
On 2014/03/12 18:17:56, Alan Knight wrote:
> On 2014/03/06 22:20:17, kevmoo wrote:
> > On 2014/03/06 22:05:38, Emily Fortuna wrote:
> > > make static function instead of global?
> > 
> > If the static uses privates on the class or make usage more clear, sure.
> > 
> > Otherwise, having thing top-level make cross-class usage easier (don't need
to
> > qualify) and it makes dependencies clear (_getOwningLibrary doesn't need
> > anything special from Indexable)
> 
> Why static or global at all? Why not just have this as a getter or zero-arg
> method on Indexable and Library?
> 
> Library get owningLibrary =>  owner.owningLibrary
> 
> Library get owningLibrary => this;
> 
> Also Home is an Indexable that won't have an owningLibrary and may have a null
> owner. So you either need a null check or an implementation in Home as well.

Done.

https://codereview.chromium.org/180243024/diff/20001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:852: final DeclarationMirror mirror;
On 2014/03/12 18:17:56, Alan Knight wrote:
> If we're re-formatting these to have their own lines, shouldn't they also have
> comments?

Not sure what the docs should be. Reverting whitespace for now.

https://codereview.chromium.org/180243024/diff/20001/pkg/docgen/lib/docgen.da...
pkg/docgen/lib/docgen.dart:1797: annotations = _createAnnotations(mirror,
owningLibrary);
On 2014/03/12 18:17:56, Alan Knight wrote:
> On 2014/03/06 22:20:17, kevmoo wrote:
> > On 2014/03/06 22:05:38, Emily Fortuna wrote:
> > > is having a bunch of random global functions really better? I'm not
certain.
> > 
> > Random top-level methods make dependencies clear. Not using something
special
> or
> > private to a given class IMHO
> 
> I don't think it's about strict dependency requirements. This is only ever
used
> in the construction of a MirrorBased. Having it as a top-level function makes
it
> top-level noise when it's really an internal detail of MirrorBased. It would
be
> better as an instance method but I presume it can't be because it's called in
> the constructor.

Since MirrorBased is in another library and we want this private, the only
option is for it to be top-level.

If at some point the rest of the types get into the models lib, we can go back
to the static method model

https://codereview.chromium.org/180243024/diff/20001/pkg/docgen/lib/src/model...
File pkg/docgen/lib/src/models.dart (right):

https://codereview.chromium.org/180243024/diff/20001/pkg/docgen/lib/src/model...
pkg/docgen/lib/src/models.dart:1: library docgen.moedls;
On 2014/03/12 18:17:56, Alan Knight wrote:
> typo

Done.

[Emily Fortuna, 2014-03-17 16:58:35]

lgtm

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

https://codereview.chromium.org/180243024/diff/100001/pkg/docgen/lib/docgen.d...
pkg/docgen/lib/docgen.dart:865: : this.mirror = mirror,
why not (this.mirror)?

[kevmoo, 2014-03-17 17:06:35]

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

https://codereview.chromium.org/180243024/diff/100001/pkg/docgen/lib/docgen.d...
pkg/docgen/lib/docgen.dart:865: : this.mirror = mirror,
On 2014/03/17 16:58:35, Emily Fortuna wrote:
> why not (this.mirror)?

If you use this.mirror, you cannot use mirror in the 'isHidden' call.

Ask gilad :-)

[kevmoo, 2014-03-17 17:06:55]

Committed patchset #6 manually as r33753 (presubmit successful).