pkg/docgen: fix docs for --json flag, bin cleanup

pkg/docgen: fix docs for --json flag, bin cleanup

R=efortuna@google.com

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

[kevmoo, 2014-02-16 00:44:45]

Fixed arg comment for --json

...and remove top-level state from the binary

[Alan Knight, 2014-02-18 18:51:54]

https://codereview.chromium.org/168743003/diff/40001/pkg/docgen/bin/docgen.dart
File pkg/docgen/bin/docgen.dart (right):

https://codereview.chromium.org/168743003/diff/40001/pkg/docgen/bin/docgen.da...
pkg/docgen/bin/docgen.dart:37: var excludedLibraries =
options[_OPTION_EXCLUDE_LIB];
This complicates main that much more and doesn't seem like it's adding value
over having a callback in the option.

https://codereview.chromium.org/168743003/diff/40001/pkg/docgen/bin/docgen.da...
pkg/docgen/bin/docgen.dart:83: String _getDefaultStartPage(files) {
I don't like "get" as a prefix. How about defaultStartPageFor(files)

https://codereview.chromium.org/168743003/diff/40001/pkg/docgen/bin/docgen.da...
pkg/docgen/bin/docgen.dart:106: help: 'Outputs to JSON. If negated, files are
outputted to YAML. '
If we're fixing this, how about fixing outputted.

https://codereview.chromium.org/168743003/diff/40001/pkg/docgen/bin/docgen.da...
pkg/docgen/bin/docgen.dart:139: parser.addOption(_OPTION_EXCLUDE_LIB,
If we're putting these in constants it should be all or none.

[Emily Fortuna, 2014-02-18 19:32:09]

I stand by all of Alan's comments.

https://codereview.chromium.org/168743003/diff/40001/pkg/docgen/bin/docgen.dart
File pkg/docgen/bin/docgen.dart (right):

https://codereview.chromium.org/168743003/diff/40001/pkg/docgen/bin/docgen.da...
pkg/docgen/bin/docgen.dart:83: String _getDefaultStartPage(files) {
On 2014/02/18 18:51:54, Alan Knight wrote:
> I don't like "get" as a prefix. How about defaultStartPageFor(files)

+1

https://codereview.chromium.org/168743003/diff/40001/pkg/docgen/bin/docgen.da...
pkg/docgen/bin/docgen.dart:139: parser.addOption(_OPTION_EXCLUDE_LIB,
On 2014/02/18 18:51:54, Alan Knight wrote:
> If we're putting these in constants it should be all or none.

+1

https://codereview.chromium.org/168743003/diff/40001/pkg/docgen/bin/docgen.da...
pkg/docgen/bin/docgen.dart:160: const _OPTION_EXCLUDE_LIB = 'exclude-lib';
put constants up at top, please

[kevmoo, 2014-02-18 19:36:06]

PTAL

https://codereview.chromium.org/168743003/diff/40001/pkg/docgen/bin/docgen.dart
File pkg/docgen/bin/docgen.dart (right):

https://codereview.chromium.org/168743003/diff/40001/pkg/docgen/bin/docgen.da...
pkg/docgen/bin/docgen.dart:37: var excludedLibraries =
options[_OPTION_EXCLUDE_LIB];
On 2014/02/18 18:51:54, Alan Knight wrote:
> This complicates main that much more and doesn't seem like it's adding value
> over having a callback in the option.

The upside is eliminating global state, IMHO. It's clear when the value is set
and where it's going.

https://codereview.chromium.org/168743003/diff/40001/pkg/docgen/bin/docgen.da...
pkg/docgen/bin/docgen.dart:83: String _getDefaultStartPage(files) {
On 2014/02/18 18:51:54, Alan Knight wrote:
> I don't like "get" as a prefix. How about defaultStartPageFor(files)

Done.

https://codereview.chromium.org/168743003/diff/40001/pkg/docgen/bin/docgen.da...
pkg/docgen/bin/docgen.dart:106: help: 'Outputs to JSON. If negated, files are
outputted to YAML. '
On 2014/02/18 18:51:54, Alan Knight wrote:
> If we're fixing this, how about fixing outputted.

Done.

https://codereview.chromium.org/168743003/diff/40001/pkg/docgen/bin/docgen.da...
pkg/docgen/bin/docgen.dart:139: parser.addOption(_OPTION_EXCLUDE_LIB,
On 2014/02/18 18:51:54, Alan Knight wrote:
> If we're putting these in constants it should be all or none.

Done.

[Emily Fortuna, 2014-02-18 21:07:05]

lgtm, but see my comment

https://codereview.chromium.org/168743003/diff/40001/pkg/docgen/bin/docgen.dart
File pkg/docgen/bin/docgen.dart (right):

https://codereview.chromium.org/168743003/diff/40001/pkg/docgen/bin/docgen.da...
pkg/docgen/bin/docgen.dart:37: var excludedLibraries =
options[_OPTION_EXCLUDE_LIB];
On 2014/02/18 19:36:06, kevmoo wrote:
> On 2014/02/18 18:51:54, Alan Knight wrote:
> > This complicates main that much more and doesn't seem like it's adding value
> > over having a callback in the option.
> 
> The upside is eliminating global state, IMHO. It's clear when the value is set
> and where it's going.

Generally removing global state is good, but I'd argue this file is much more in
the vein of a sort of  one-off starter script, where eliminating globals is not
really a big deal (if this were a library I'd feel differently). That said. I
don't care all that strongly.

[kevmoo, 2014-02-18 21:08:28]

Alan?

https://codereview.chromium.org/168743003/diff/40001/pkg/docgen/bin/docgen.dart
File pkg/docgen/bin/docgen.dart (right):

https://codereview.chromium.org/168743003/diff/40001/pkg/docgen/bin/docgen.da...
pkg/docgen/bin/docgen.dart:37: var excludedLibraries =
options[_OPTION_EXCLUDE_LIB];
Another point: this is the only spot where a global is used for these setting.
The rest (see above) are accessed locally.

[kevmoo, 2014-02-19 03:07:30]

Committed patchset #3 manually as r32774 (presubmit successful).