code review updates

jakemac53 · jakemac53 · commit 1dd10145b150 · 2016-01-26T08:59:22.000-08:00
diff --git a/e2e_example/lib/copy_builder.dart b/e2e_example/lib/copy_builder.dart
@@ -1,8 +1,6 @@
 // Copyright (c) 2016, 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.
-library build.example.copy_builder;
-
 import 'dart:async';
 
 import 'package:build/build.dart';
@@ -12,7 +10,7 @@ class CopyBuilder extends Builder {
   @override
   Future build(BuildStep buildStep) async {
     var input = buildStep.input;
-    var copy = new Asset(_copiedAssetId(input.id), input.contents);
+    var copy = new Asset(_copiedAssetId(input.id), input.stringContents);
     await buildStep.writeAsString(copy);
   }
 
diff --git a/e2e_example/lib/transformer.dart b/e2e_example/lib/transformer.dart
@@ -1,8 +1,6 @@
 // Copyright (c) 2016, 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.
-library build.example.generate;
-
 import 'package:build/build.dart';
 
 import 'copy_builder.dart';
diff --git a/e2e_example/tool/build.dart b/e2e_example/tool/build.dart
@@ -1,8 +1,6 @@
 // Copyright (c) 2016, 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.
-library build.example.generate;
-
 import 'package:build/build.dart';
 
 import 'package:e2e_example/copy_builder.dart';
diff --git a/lib/build.dart b/lib/build.dart
@@ -1,8 +1,6 @@
 // Copyright (c) 2016, 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.
-library build;
-
 export 'src/asset/asset.dart';
 export 'src/asset/id.dart';
 export 'src/builder/build_step.dart';
diff --git a/lib/src/asset/asset.dart b/lib/src/asset/asset.dart
@@ -1,8 +1,6 @@
 // Copyright (c) 2016, 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.
-library build.src.asset.asset;
-
 import 'id.dart';
 
 /// A fully realized asset whose content is available synchronously.
@@ -11,9 +9,9 @@ class Asset {
   final AssetId id;
 
   /// The content for this asset.
-  final String contents;
+  final String stringContents;
 
-  Asset(this.id, this.contents);
+  Asset(this.id, this.stringContents);
 
   String toString() => 'Asset: $id';
 }
diff --git a/lib/src/asset/id.dart b/lib/src/asset/id.dart
@@ -1,8 +1,6 @@
 // Copyright (c) 2016, 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.
-library build.src.asset.id;
-
 import 'package:path/path.dart' as pathos;
 
 /// Forked from the `barback` package.
diff --git a/lib/src/asset/reader.dart b/lib/src/asset/reader.dart
@@ -1,8 +1,6 @@
 // Copyright (c) 2016, 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.
-library build.src.asset.reader;
-
 import 'dart:async';
 import 'dart:convert';
 
diff --git a/lib/src/asset/writer.dart b/lib/src/asset/writer.dart
@@ -1,8 +1,6 @@
 // Copyright (c) 2016, 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.
-library build.src.asset.writer;
-
 import 'dart:async';
 import 'dart:convert';
 
diff --git a/lib/src/builder/build_step.dart b/lib/src/builder/build_step.dart
@@ -1,8 +1,6 @@
 // Copyright (c) 2016, 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.
-library build.src.builder.build_step;
-
 import 'dart:async';
 import 'dart:collection';
 import 'dart:convert';
diff --git a/lib/src/builder/builder.dart b/lib/src/builder/builder.dart
@@ -1,8 +1,6 @@
 // Copyright (c) 2016, 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.
-library build.src.builder.builder;
-
 import 'dart:async';
 
 import '../asset/id.dart';
diff --git a/lib/src/generate/build.dart b/lib/src/generate/build.dart
@@ -1,8 +1,6 @@
 // Copyright (c) 2016, 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.
-library build.src.generate.generate;
-
 import 'dart:async';
 import 'dart:convert';
 import 'dart:io';
@@ -56,11 +54,8 @@ void _validatePhases(List<List<Phase>> phaseGroups) {
   }
 }
 
-/// Validates the phases, for now that just means making sure they are all only
-/// running on the current package.
-///
-/// Returns a [Future<BuildResult>] which completes once all [Phase]s are done
-/// running.
+/// Runs the [phaseGroups] and returns a [Future<BuildResult>] which completes
+/// once all [Phase]s are done.
 Future<BuildResult> _runPhases(List<List<Phase>> phaseGroups) async {
   var outputs = [];
   for (var group in phaseGroups) {
@@ -69,7 +64,9 @@ Future<BuildResult> _runPhases(List<List<Phase>> phaseGroups) async {
       for (var builder in phase.builders) {
         // TODO(jakemac): Optimize, we can run all the builders in a phase
         // at the same time instead of sequentially.
-        await _runBuilder(builder, inputs, outputs);
+        await for (var output in _runBuilder(builder, inputs)) {
+          outputs.add(output);
+        }
       }
     }
   }
@@ -99,55 +96,58 @@ Set<File> _filesMatching(InputSet inputSet) {
   }
 
   var files = new Set<File>();
-  for (var pattern in inputSet.filePatterns) {
-    files.addAll(new Glob(pattern).listSync(followLinks: false).where(
+  for (var glob in inputSet.globs) {
+    files.addAll(glob.listSync(followLinks: false).where(
         (e) => e is File && !_ignoredDirs.contains(path.split(e.path)[1])));
   }
   return files;
 }
 
 /// Runs [builder] with [inputs] as inputs.
-Future _runBuilder(
-    Builder builder, List<AssetId> inputs, List<Asset> outputs) async {
+Stream<Asset> _runBuilder(Builder builder, List<AssetId> inputs) async* {
   for (var input in inputs) {
     var expectedOutputs = builder.declareOutputs(input);
     var inputAsset = new Asset(input, await _reader.readAsString(input));
     var buildStep =
         new BuildStep(inputAsset, expectedOutputs, _reader, _writer);
     await builder.build(buildStep);
     await buildStep.outputsCompleted;
-    outputs.addAll(buildStep.outputs);
+    for (var output in buildStep.outputs) {
+      yield output;
+    }
   }
 }
 
 /// Very simple [AssetReader], only works on local package and assumes you are
 /// running from the root of the package.
 class _SimpleAssetReader implements AssetReader {
+  const _SimpleAssetReader();
+
   @override
   Future<String> readAsString(AssetId id, {Encoding encoding: UTF8}) async {
     assert(id.package == _localPackageName);
     return new File(id.path).readAsString(encoding: encoding);
   }
 }
 
-AssetReader _reader = new _SimpleAssetReader();
+const AssetReader _reader = const _SimpleAssetReader();
 
 /// Very simple [AssetWriter], only works on local package and assumes you are
 /// running from the root of the package.
 class _SimpleAssetWriter implements AssetWriter {
   final _outputDir;
 
-  _SimpleAssetWriter(this._outputDir);
+  const _SimpleAssetWriter(this._outputDir);
 
   @override
   Future writeAsString(Asset asset, {Encoding encoding: UTF8}) async {
     assert(asset.id.package == _localPackageName);
     var file = new File(path.join(_outputDir, asset.id.path));
     await file.create(recursive: true);
-    await file.writeAsString(asset.contents, encoding: encoding);
+    await file.writeAsString(asset.stringContents, encoding: encoding);
   }
 }
 
-AssetWriter _writer = new _SimpleAssetWriter('generated');
+const AssetWriter _writer = const _SimpleAssetWriter('generated');
 
 const _ignoredDirs = const ['generated', 'build', 'packages'];
diff --git a/lib/src/generate/build_result.dart b/lib/src/generate/build_result.dart
@@ -1,8 +1,6 @@
 // Copyright (c) 2016, 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.
-library build.src.builder.build_result;
-
 import '../asset/asset.dart';
 
 /// The result of an individual build, this may be an incremental build or
diff --git a/lib/src/generate/input_set.dart b/lib/src/generate/input_set.dart
@@ -1,22 +1,22 @@
 // Copyright (c) 2016, 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.
-library build.src.generate.input_set;
+import 'package:glob/glob.dart';
 
 /// Represents a set of files in a package which should be used as primary
 /// inputs to a `Builder`.
 class InputSet {
   /// The package that the [globs] should be ran on.
   final String package;
 
-  /// The file patterns from [package] to use as inputs, glob syntax is
-  /// supported.
+  /// The [Glob]s for files from [package] to use as inputs.
   ///
   /// Note: If the [package] is a package dependency, then only files under
   /// `lib` will be available, but for the application package any files can be
   /// listed.
-  final List<String> filePatterns;
+  final List<Glob> globs;
 
   InputSet(this.package, {Iterable<String> filePatterns})
-      : this.filePatterns = new List.unmodifiable(filePatterns);
+      : this.globs = new List.unmodifiable(
+            filePatterns.map((pattern) => new Glob(pattern)));
 }
diff --git a/lib/src/generate/phase.dart b/lib/src/generate/phase.dart
@@ -1,14 +1,12 @@
 // Copyright (c) 2016, 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.
-library build.src.generate.phase;
-
 import '../builder/builder.dart';
 import 'input_set.dart';
 
 /// A single phase in the build process. None of the [Builder]s in a single
 /// phase should depend on any of the outputs of other [Builder]s in that same
-/// phase. 
+/// phase.
 class Phase {
   /// The list of all [Builder]s that should be run on all [InputSet]s.
   final List<Builder> builders;
diff --git a/lib/src/package_graph/package_graph.dart b/lib/src/package_graph/package_graph.dart
@@ -1,11 +1,8 @@
 // Copyright (c) 2016, 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.
-library build.src.package_graph.package_graph;
-
 import 'dart:io';
 
-import 'package:path/path.dart' as path;
 import 'package:yaml/yaml.dart';
 
 /// A graph of the package dependencies for an application.
@@ -34,12 +31,14 @@ class PackageGraph {
   /// Creates a [PackageGraph] for the package in which you are currently
   /// running.
   factory PackageGraph.forThisPackage() {
+    /// Read in the pubspec file and parse it as yaml.
     var pubspec = new File('pubspec.yaml');
     if (!pubspec.existsSync()) {
       throw 'Unable to generate package graph, no `pubspec.yaml` found.';
     }
     var yaml = loadYaml(pubspec.readAsStringSync());
 
+    /// Read in the `.packages` file to get the locations of all packages.
     var packagesFile = new File('.packages');
     if (!packagesFile.existsSync()) {
       throw 'Unable to generate package graph, no `.packages` found.';
@@ -58,11 +57,28 @@ class PackageGraph {
       }
     });
 
+    /// Create all [PackageNode]s for all deps.
     var nodes = <String, PackageNode>{};
-    var root = new PackageNode.forPubspec(
-        yaml, PackageDependencyType.Path, nodes, packageLocations,
-        isRoot: true);
+    PackageNode addNodeAndDeps(YamlMap yaml, PackageDependencyType type,
+        {bool isRoot: false}) {
+      var name = yaml['name'];
+      assert(!nodes.containsKey(name));
+      var node = new PackageNode._(name, yaml['version'], type);
+      nodes[name] = node;
+
+      _depsFromYaml(yaml, withOverrides: isRoot).forEach((name, source) {
+        var dep = nodes[name];
+        if (dep == null) {
+          var pubspec = _pubspecForUri(packageLocations[name]);
+          dep = addNodeAndDeps(pubspec, _dependencyType(source));
+        }
+        node._dependencies.add(dep);
+      });
+
+      return node;
+    }
 
+    var root = addNodeAndDeps(yaml, PackageDependencyType.Path, isRoot: true);
     return new PackageGraph._(root);
   }
 
@@ -77,39 +93,21 @@ class PackageGraph {
 
 /// A node in a [PackageGraph].
 class PackageNode {
+  /// The name of the package as listed in the pubspec.yaml
   final String name;
 
+  /// The version of the package as listed in the pubspec.yaml
   final String version;
 
+  /// The type of dependency being used to pull in this package.
   final PackageDependencyType dependencyType;
 
+  /// All the packages that this package directly depends on.
   final List<PackageNode> _dependencies = [];
   Iterable<PackageNode> get dependencies => _dependencies.toList();
 
   PackageNode._(this.name, this.version, this.dependencyType);
 
-  factory PackageNode.forPubspec(YamlMap yaml, PackageDependencyType type,
-      Map<String, PackageNode> nodes, Map<String, Uri> packageLocations,
-      {bool isRoot: false}) {
-    var node = new PackageNode._(yaml['name'], yaml['version'], type);
-    assert(!nodes.containsKey(node.name));
-    nodes[node.name] = node;
-    var deps = _depsFromYaml(yaml, withOverrides: isRoot);
-
-    deps.forEach((name, source) {
-      var dep = nodes[name];
-      if (dep == null) {
-        var pubspec = _pubspecForUri(packageLocations[name]);
-        dep = new PackageNode.forPubspec(
-            pubspec, _dependencyType(source), nodes, packageLocations);
-        nodes[name] = dep;
-      }
-      node._dependencies.add(dep);
-    });
-
-    return node;
-  }
-
   String toString() => '''
   $name:
     version: $version
diff --git a/lib/src/transformer/transformer.dart b/lib/src/transformer/transformer.dart
@@ -1,8 +1,6 @@
 // Copyright (c) 2016, 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.
-library build.src.transformer.transformer;
-
 import 'dart:async';
 import 'dart:convert';
 
@@ -89,7 +87,8 @@ build.AssetId _toBuildAssetId(barback.AssetId id) =>
     new build.AssetId(id.package, id.path);
 
 barback.Asset _toBarbackAsset(build.Asset asset) =>
-    new barback.Asset.fromString(_toBarbackAssetId(asset.id), asset.contents);
+    new barback.Asset.fromString(
+        _toBarbackAssetId(asset.id), asset.stringContents);
 
 Future<build.Asset> _toBuildAsset(barback.Asset asset) async =>
     new build.Asset(_toBuildAssetId(asset.id), await asset.readAsString());
