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

pkg/docgen/bin/docgen.dart

Index: pkg/docgen/bin/docgen.dart
diff --git a/pkg/docgen/bin/docgen.dart b/pkg/docgen/bin/docgen.dart
index f89162a3d061578ee45e432fce014961fbe7a305..232dbcb2ad24df1f5a4b5aa1f9189f702964db2e 100644
--- a/pkg/docgen/bin/docgen.dart
+++ b/pkg/docgen/bin/docgen.dart
@@ -6,16 +6,9 @@ import 'dart:io';
 
 import 'package:args/args.dart';
 import 'package:logging/logging.dart';
-
-import '../lib/docgen.dart';
 import 'package:path/path.dart' as path;
 
-List<String> excludedLibraries = [];
-
-/**
- * The files/directories that we're being asked to document.
- */
-List<String> _files;
+import '../lib/docgen.dart';
 
 /**
  * Analyzes Dart files and generates a representation of included libraries,
@@ -23,11 +16,11 @@ List<String> _files;
  */
 void main(List<String> arguments) {
   var options = _initArgParser().parse(arguments);
-  _files = options.rest.map(path.normalize).toList();
-  if (_files.isEmpty) _printHelpAndExit();
+  var files = options.rest.map(path.normalize).toList();
+  if (files.isEmpty) _printHelpAndExit();
   var startPage = options['start-page'];
-  if (_singlePackage(_files) && startPage == null) {
-    startPage = _defaultStartPage;
+  if (_singlePackage(files) && startPage == null) {
+    startPage = _getDefaultStartPage(files);
     print("Using default options for documenting a single package: "
         "--start-page=$startPage");
   }
@@ -35,13 +28,16 @@ void main(List<String> arguments) {
   var scriptDir = path.dirname(Platform.script.toFilePath());
   var introduction = includeSdk ? '' : options['introduction'];
 
-  var pubScript = options['sdk'] != null ? 
+  var pubScript = options['sdk'] != null ?
       path.join(options['sdk'], 'bin', 'pub') : 'pub';
 
-  var dartBinary = options['sdk'] != null ? 
+  var dartBinary = options['sdk'] != null ?
       path.join(options['sdk'], 'bin', 'dart') : 'dart';
 
-  docgen(_files,
+  var excludedLibraries = options[_OPTION_EXCLUDE_LIB];

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

This complicates main that much more and doesn't seem like it's adding value
over having a callback in the option.

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

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

[Emily Fortuna, 2014/02/18 21:07:06]

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]

Another point: this is the only spot where a global is used for these setting.
The rest (see above) are accessed locally.

+  if(excludedLibraries == null) excludedLibraries = [];
+
+  docgen(files,
       packageRoot: options['package-root'],
       outputToYaml: !options['json'],
       includePrivate: options['include-private'],
@@ -84,10 +80,10 @@ bool _singlePackage(List files) {
  * If we've specified just a package and no other command-line options,
  * use the single package name as the start page.
  */
-String get _defaultStartPage {
-    var pubspec = new File(path.join(_files.first, 'pubspec.yaml'));
-    if (!pubspec.existsSync()) return null;
-    return Library.packageNameFor(_files.first);
+String _getDefaultStartPage(files) {

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

I don't like "get" as a prefix. How about defaultStartPageFor(files)

[Emily Fortuna, 2014/02/18 19:32:10]

+1

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

Done.

+  var pubspec = new File(path.join(files.first, 'pubspec.yaml'));
+  if (!pubspec.existsSync()) return null;
+  return Library.packageNameFor(files.first);
 }
 
 /**
@@ -107,9 +103,9 @@ ArgParser _initArgParser() {
         if (verbose) Logger.root.level = Level.FINEST;
       });
   parser.addFlag('json', abbr: 'j',
-      help: 'Outputs to JSON. Files are outputted to YAML by default. '
+      help: 'Outputs to JSON. If negated, files are outputted to YAML. '

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

If we're fixing this, how about fixing outputted.

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

Done.

         'If --append is used, it takes the file-format of the previous '
-        'run stated in library_list.json ignoring the flag.',
+        'run stated in library_list.json, ignoring the flag.',
       negatable: true, defaultsTo: true);
   parser.addFlag('include-private',
       help: 'Flag to include private declarations.', negatable: false);
@@ -140,10 +136,9 @@ ArgParser _initArgParser() {
   parser.addOption('out',
       help: 'The name of the output directory.',
       defaultsTo: 'docs');
-  parser.addOption('exclude-lib',
+  parser.addOption(_OPTION_EXCLUDE_LIB,

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

If we're putting these in constants it should be all or none.

[Emily Fortuna, 2014/02/18 19:32:10]

+1

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

Done.

       help: 'Exclude the library by this name from the documentation',
-      allowMultiple: true,
-      callback: (libs) => excludedLibraries.addAll(libs));
+      allowMultiple: true);
   parser.addFlag('include-dependent-packages',
       help: 'Assumes we are documenting a single package and are running '
         'in the directory with its pubspec. Includes documentation for all '
@@ -153,7 +148,7 @@ ArgParser _initArgParser() {
       help: 'SDK directory',
       defaultsTo: null);
   parser.addOption('start-page',
-      help: 'By default the viewer will start at the SDK introduction page.'
+      help: 'By default the viewer will start at the SDK introduction page. '
         'To start at some other page, e.g. for a package, provide the name '
         'of the package in this argument, e.g. --start-page=intl will make '
         'the start page of the viewer be the intl package.',
@@ -161,3 +156,6 @@ ArgParser _initArgParser() {
 
   return parser;
 }
+
+const _OPTION_EXCLUDE_LIB = 'exclude-lib';

[Emily Fortuna, 2014/02/18 19:32:10]

put constants up at top, please

+