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

pkg/docgen/bin/docgen.dart

 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 import 'dart:io';
 
 import 'package:args/args.dart';
 import 'package:logging/logging.dart';
+import 'package:path/path.dart' as path;
 
 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;
 
 /**
  * Analyzes Dart files and generates a representation of included libraries,
  * classes, and members.
  */
 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");
   }
   var includeSdk = options['parse-sdk'] || options['include-sdk'];
   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'],
       includeSdk: includeSdk,
       parseSdk: options['parse-sdk'],
       append: options['append'] && new Directory(options['out']).existsSync(),
       introFileName: introduction,
       out: options['out'],
       excludeLibraries: excludedLibraries,
       includeDependentPackages: options['include-dependent-packages'],
@@ skipping 22 matching lines @@
   if (files.length != 1) return false;
   var pubspec = new File(path.join(files.first, 'pubspec.yaml'));
   if (!pubspec.existsSync()) return false;
   return true;
 }
 
 /**
  * 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);
 }
 
 /**
  * Creates parser for docgen command line arguments.
  */
 ArgParser _initArgParser() {
   var parser = new ArgParser();
   parser.addFlag('help', abbr: 'h',
       help: 'Prints help and usage information.',
       negatable: false,
       callback: (help) {
         if (help) _printHelpAndExit();
       });
   parser.addFlag('verbose', abbr: 'v',
       help: 'Output more logging information.', negatable: false,
       callback: (verbose) {
         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);
   parser.addFlag('include-sdk',
       help: 'Flag to parse SDK Library files.',
       defaultsTo: true,
       negatable: true);
   parser.addFlag('parse-sdk',
       help: 'Parses the SDK libraries only.',
       defaultsTo: false, negatable: false);
@@ skipping 10 matching lines @@
       'and start a simple server',
       defaultsTo: false, negatable: false);
   parser.addFlag('no-docs', help: 'Do not generate any new documentation',
       defaultsTo: false, negatable: false);
   parser.addOption('introduction',
       help: 'Adds the provided markdown text file as the introduction'
         ' for the generated documentation.', defaultsTo: '');
   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 '
         'of its dependent packages.',
       defaultsTo: true, negatable: true);
   parser.addOption('sdk',
       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.',
         defaultsTo: null);
 
   return parser;
 }
+
+const _OPTION_EXCLUDE_LIB = 'exclude-lib';

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

put constants up at top, please

+