Show detailed report on upgrade.

sdk/lib/_internal/pub/lib/src/solver/solve_report.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.
+
+library pub.solver.solve_report;
+
+import '../lock_file.dart';
+import '../log.dart' as log;
+import '../package.dart';
+import '../source_registry.dart';
+import '../utils.dart';
+import 'version_solver.dart';
+
+/// Generates and displays nicely formatted reports for the results of running
+/// a version resolution.
+class SolveReport {

[nweiz, 2013/12/11 06:48:32]

At first glance the fact that this is different than SolveResult is pretty
confusing.

I'm also not a big fan of classes that just expose a single method that's only
intended to be called once. It seems cleaner just to make this a function
instead, even if internally it's implemented as a class.

[Bob Nystrom, 2013/12/11 22:36:59]

Added some more docs clarifying.
I can do that if you want, but it doesn't add much value to me. This library is
only imported by version_solver.dart and it only exports the class.

[nweiz, 2013/12/11 23:31:27]

I do. Exporting a class instead of a function implies that the class will be
used like a class, which is confusing since it isn't.

[Bob Nystrom, 2013/12/12 00:14:58]

Done.

+  final SourceRegistry _sources;
+  final Package _root;
+  final LockFile _previousLockFile;
+  final SolveResult _result;
+
+  /// The dependencies in [_result], keyed by package name.
+  final _dependencies = new Map<String, PackageId>();
+
+  final _output = new StringBuffer();
+
+  /// To avoid emitting trailing newlines, we track if any are needed and only
+  /// emit then when text on the next line is about to be written.
+  int _pendingLines = 0;

[nweiz, 2013/12/11 06:48:32]

This is pretty general behavior. It would be nice to implement it as a wrapper
for StringSink. That would also make this class a little less bulky.

[Bob Nystrom, 2013/12/11 22:36:59]

Added a TODO. I like the idea, but I can't come up with a good name for the
wrapper, and we don't have any other uses for it yet, so I'd like to postpone it
until we need it.

[nweiz, 2013/12/11 23:31:27]

I'm not a big fan of the philosophy of "postpone the more general implementation
until we have two+ uses for it", but w/e.

+
+  SolveReport(this._sources, this._root, this._previousLockFile, this._result) {
+    // Fill the map so we can use it later.
+    for (var id in _result.packages) {
+      _dependencies[id.name] = id;
+    }
+  }
+
+  /// Displays a report of the results of the version resolution relative to
+  /// the previous lock file.
+  ///
+  /// If [showAll] is true, then all of the previous and current dependencies
+  /// are shown and their changes relative to the previous lock file are
+  /// highlighted. Otherwise, only overrides are shown.
+  ///
+  /// Returns the number of changed dependencies.
+  int show({bool showAll}) {
+    if (showAll) _reportChanges();
+    _reportOverrides();
+
+    // Count how many dependencies actually changed.
+    var dependencies = _dependencies.keys.toSet();
+    dependencies.addAll(_previousLockFile.packages.keys);
+    dependencies.remove(_root.name);
+
+    return dependencies.where((name) {
+      var oldId = _previousLockFile.packages[name];
+      var newId = _dependencies[name];
+
+      // Added or removed dependencies count.
+      if (oldId == null) return true;
+      if (newId == null) return true;
+
+      // The dependency existed before, so see if it was modified.
+      return !_descriptionsEqual(oldId, newId) ||
+          oldId.version != newId.version;
+    }).length;
+  }
+
+  /// Displays a report of all of the previous and current dependencies and
+  /// how they have changed.
+  void _reportChanges() {
+    _output.clear();

[nweiz, 2013/12/11 06:48:32]

Clearing and writing to a StringBuffer instance variable seems kind of gross to
me. I think it would be cleaner if _reportPackage just took a StringSink as a
parameter.

[Bob Nystrom, 2013/12/11 22:36:59]

It's a bit gross to me too, but this class's reason for being is to minimize how
many parameters have to be passed around to its different methods so I figured
it made sense to lean on that.

+
+    // Show the current dependencies ordered by name.

[nweiz, 2013/12/11 06:48:32]

"current" -> "new"

[Bob Nystrom, 2013/12/11 22:36:59]

I used "current" here because "new" could mean "dependencies that were just
added" and this includes existing ones as well.

[nweiz, 2013/12/11 23:31:27]

"current" is confusing because it's not clear which dependencies at which point
in the process are being referred to. Is this running after the versions have
been solved but before the dependencies have been installed? What would the
"current" dependencies refer to in that case?

"new set of dependencies" might make it clearer that you aren't just referring
to the dependencies that didn't exist before.

[Bob Nystrom, 2013/12/12 00:14:58]

Done.

+    var names = _result.packages.map((id) => id.name).toList();
+    names.remove(_root.name);
+    names.sort();
+    names.forEach(_reportPackage);
+
+    // Show any removed ones.
+    var removed = _previousLockFile.packages.keys.toSet();
+    removed.removeAll(names);
+    if (removed.isNotEmpty) {
+      _writeln("These packages are no longer being depended on:");
+      removed = removed.toList();
+      removed.sort();
+      removed.forEach(_reportPackage);
+    }
+
+    log.message(_output.toString());
+  }
+
+  /// Displays a warning about the overrides currently in effect.
+  void _reportOverrides() {
+    _output.clear();
+
+    if (_result.overrides.isNotEmpty) {
+      _writeln("Warning: You are using these overridden dependencies:");
+      var overrides = _result.overrides.map((dep) => dep.name).toList();
+      overrides.sort((a, b) => a.compareTo(b));
+
+      overrides.forEach(
+          (name) => _reportPackage(name, highlightOverride: false));
+
+      log.warning(_output.toString());
+    }
+  }
+
+  /// Reports the results of the upgrade on the package named [name].
+  ///
+  /// If [highlightOverride] is true (or absent), writes "(override)" next to
+  /// overridden packages.
+  void _reportPackage(String name, {bool highlightOverride}) {
+    if (highlightOverride == null) highlightOverride = true;

[nweiz, 2013/12/11 06:48:32]

Blah blah explicit default.

[Bob Nystrom, 2013/12/11 22:36:59]

Previous comment.

+
+    var newId = _dependencies[name];
+    var oldId = _previousLockFile.packages[name];
+    var id = newId != null ? newId : oldId;
+
+    var isOverridden = _result.overrides.map(
+        (dep) => dep.name).contains(id.name);
+
+    var changed = false;
+
+    // Show a one-character "icon" describing the change. They are:
+    //
+    //     ! The package is being overridden.
+    //     - The package was removed.
+    //     + The package was added.
+    //     > The package was upgraded from a lower version.
+    //     < The package was downgraded from a higher version.
+    //     * Any other change between the old and new package.
+    if (isOverridden) {
+      _write(log.magenta("! "));
+    } else if (newId == null) {
+      _write(log.red("- "));
+    } else if (oldId == null) {
+      _write(log.green("+ "));
+    } else if (!_descriptionsEqual(oldId, newId)) {
+      _write(log.cyan("* "));

[nweiz, 2013/12/11 06:48:32]

For colorblind folks like myself, it might be nice to have a comment somewhere
explaining when you chose to use magenta vs cyan.

[Bob Nystrom, 2013/12/11 22:36:59]

Added a bunch of docs to the color functions in log.

+      changed = true;
+    } else if (oldId.version < newId.version) {
+      _write(log.green("> "));
+      changed = true;
+    } else if (oldId.version > newId.version) {
+      _write(log.cyan("< "));
+      changed = true;
+    } else {
+      // Unchanged.
+      _write("  ");
+    }
+
+    // If the package was upgraded, show what it was upgraded from.

[nweiz, 2013/12/11 06:48:32]

This comment should be on the next block.

[Bob Nystrom, 2013/12/11 22:36:59]

Done.

+    _write(log.bold(id.name));
+    _write(" ");
+    _writeId(id);
+
+    if (changed) {
+      _write(" (was ");
+      _writeId(oldId);
+      _write(")");
+    }
+
+    // Highlight overridden packages.
+    if (isOverridden && highlightOverride) {
+      _write(" ${log.magenta('(overridden)')}");
+    }
+
+    // See if there are any newer versions of the package that we were
+    // unable to upgrade to.
+    if (newId != null) {
+      var versions = _result.availableVersions[newId.name];
+      var newerStable = 0;
+      var newerUnstable = 0;
+
+      for (var version in versions) {
+        if (version > newId.version) {
+          if (version.isPreRelease) {
+            newerUnstable++;
+          } else {
+            newerStable++;
+          }
+        }
+      }
+
+      // If there are newer stable versions, only show those.
+      var message;
+      if (newerStable > 0) {
+        message = "($newerStable newer "
+            "${pluralize('version', newerStable)} available)";
+      } else if (newerUnstable > 0) {
+        message = "($newerUnstable newer unstable "
+            "${pluralize('version', newerUnstable)} available)";
+      }
+
+      if (message != null) _write(" ${log.cyan(message)}");
+    }
+
+    _writeln();
+  }
+
+  /// Returns `true` if [a] and [b] are from the same source and have the same
+  /// description.
+  bool _descriptionsEqual(PackageId a, PackageId b) {
+    if (a.source != b.source) return false;
+
+    if (_sources.contains(a.source)) {
+      var source = _sources[a.source];
+      return source.descriptionsEqual(a.description, b.description);
+    } else {
+      // Unknown source, so just do a literal comparison.

[nweiz, 2013/12/11 06:48:32]

If it's an unknown source there's no way the version resolution would have
succeeded. I don't think we need to handle this case gracefully here. Same goes
for _writeId.

[Bob Nystrom, 2013/12/11 22:36:59]

I added a TODO elsewhere for this. What I want to do in a later patch is use the
"null object" pattern for unknown sources. sources[...] will always return a
valid Source object, it may just be an instance of UnknownSource. That will do
"safe" things for most operations and let the caller know for cases where it
shouldn't be used. That will clean up this and a lot of other code.

+      return a.description == b.description;
+    }
+  }
+
+  /// Writes a terse description of [id] (not including its name) to the output.
+  void _writeId(PackageId id) {
+    _write(id.version);
+
+    var source = null;
+    if (_sources.contains(id.source)) {
+      source = _sources[id.source];
+    }
+
+    if (source != null && source != _sources.defaultSource) {
+      var description = source.formatDescription(_root.dir, id.description);
+      _write(" from ${id.source} $description");
+    }
+  }
+
+  /// Writes [obj] to the output.
+  void _write(Object obj) {
+    while (_pendingLines > 0) {
+      _output.writeln();
+      _pendingLines--;
+    }
+    _output.write(obj);
+  }
+
+  /// Writes [obj] (if not null) followed by a newline to the output.
+  ///
+  /// Doesn't actually immediately write. Instead, it waits until output is

[nweiz, 2013/12/11 06:48:32]

"immediately write a newline".

[Bob Nystrom, 2013/12/11 22:36:59]

Done.

+  /// written on the nextline. That way, trailing newlines aren't displayed.

[nweiz, 2013/12/11 06:48:32]

"next line"

[Bob Nystrom, 2013/12/11 22:36:59]

Done.

+  void _writeln([Object obj]) {
+    if (obj != null) _write(obj);
+    _pendingLines++;
+  }
+}