Show detailed report on upgrade.

Show detailed report on upgrade.

R=nweiz@google.com

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

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

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command/upgrade.dart (right):

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/upgrade.dart:37: log.message("Changed
$numChanged dependencies!");
I don't know how clear the notion of "changing a dependency" will be to users. I
assume it's because sometimes a dependency is downgraded or switched to a
different source instead of upgraded, but it would be nice if we could provide
positive-sounding output when only upgrades happened. Maybe if [getDependencies]
returned both numUpgraded and numChanged?

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:118: /// previously locked
packages.
It sounds like [useLatest] won't do anything if [upgradeAll] is true. Document
that, and maybe throw an ArgumentError if [useLatest] is non-empty and
[upgradeAll] is true.

Alternately, encode this requirement into the methods themselves by leaving this
as two separate methods.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:121: /// report of the changes
made relative to the previous lockfile.
Explain that this is a temporary situation. Ideally, reference a bug for getting
detailed reports for get/serve/build as well.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:125: Future<int>
getDependencies({List<String> useLatest, bool upgradeAll}) {
I don't think I like [getDependencies] as the name here anymore, since most of
the work it's doing is now upgrading packages and it's no longer specifically
"the method that maps to 'pub get'." It's awkward that "get" means both "install
packages so they're accessible to the app" and "non-destructively get a value
from an object", but this is running into that ambiguity and I think renaming it
would work better.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:126: if (upgradeAll == null)
upgradeAll = false;
I like using explicit defaults for boolean parameters.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart (right):

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:105: _upgradeAll =
upgradeAll == true {
Again, I think this would be much cleaner with an explicit default for
[upgradeAll].

Also, keeping this as a flag as opposed to using it to modify [lockFile] means
that you have to be sure that every time you access [lockFile] you remember to
check [_upgradeAll] as well, which seems sub-optimal.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:149:
availableVersions[package.name] = versions;
Factor this chunk out into _getAvailableVersions(PackageId package).

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:153:
availableVersions, null, attemptedSolutions);
This constructor is getting pretty unwieldy, especially with each invocation
passing null to different parameters. At least make the parameters where one or
the other invocation passes null or {} into named parameters.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/solver/solve_report.dart (right):

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:16: class SolveReport {
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.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:29: int _pendingLines =
0;
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.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:72: _output.clear();
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.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:74: // Show the current
dependencies ordered by name.
"current" -> "new"

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:114: if
(highlightOverride == null) highlightOverride = true;
Blah blah explicit default.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:140: _write(log.cyan("*
"));
For colorblind folks like myself, it might be nice to have a comment somewhere
explaining when you chose to use magenta vs cyan.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:153: // If the package
was upgraded, show what it was upgraded from.
This comment should be on the next block.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:211: // Unknown source,
so just do a literal comparison.
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.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:242: /// Doesn't actually
immediately write. Instead, it waits until output is
"immediately write a newline".

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:243: /// written on the
nextline. That way, trailing newlines aren't displayed.
"next line"

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/solver/version_solver.dart (right):

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/version_solver.dart:195: /// by [package]
Extra newline

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/test/t...
File sdk/lib/_internal/pub/test/test_pub.dart (right):

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/test_pub.dart:272: RunCommand(this.name, String
pattern)
If you're going to take a pattern, take a Pattern. Taking a string and
converting it to a RegExp implicitly is confusing.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/test/u...
File sdk/lib/_internal/pub/test/upgrade/report/describes_change_test.dart
(right):

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/test/u...
sdk/lib/_internal/pub/test/upgrade/report/describes_change_test.dart:57: .
source_changed 2\.0\.0 from path \.\./source_changed \(was 1\.0\.0\)
These "../" paths will probably break on Windows.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/test/u...
sdk/lib/_internal/pub/test/upgrade/report/describes_change_test.dart:59: .
version_changed 2\.0\.0 \(was 1\.0\.0\)
Why are the initial characters here "."s?

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/test/u...
sdk/lib/_internal/pub/test/upgrade/report/describes_change_test.dart:60: """,
multiLine: true));
This massively-escaped regexp is pretty gross and unreadable. Maybe allow
[output] to be a list of patterns, each of which is matched against an
individual line?

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

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command/upgrade.dart (right):

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/upgrade.dart:37: log.message("Changed
$numChanged dependencies!");
On 2013/12/11 06:48:32, nweiz wrote:
> I don't know how clear the notion of "changing a dependency" will be to users.
I
> assume it's because sometimes a dependency is downgraded or switched to a
> different source instead of upgraded,

Yup, also the common cases of adding a new dependency or removing one.

> but it would be nice if we could provide
> positive-sounding output when only upgrades happened. Maybe if
[getDependencies]
> returned both numUpgraded and numChanged?

I added a TODO. We can improve this, but at some point we're just gilding the
lily. I think the most important point (and the one users have asked for) is
that we tell them if nothing changed. This covers that. The report itself covers
the other stuff.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:118: /// previously locked
packages.
On 2013/12/11 06:48:32, nweiz wrote:
> It sounds like [useLatest] won't do anything if [upgradeAll] is true. Document
> that, and maybe throw an ArgumentError if [useLatest] is non-empty and
> [upgradeAll] is true.

They are actually orthogonal. useLatest *forces* the latest version and *only*
the latest version. upgradeAll just unlocks. That was the semantics you
originally implemented for pub upgrade, but I'm not sure I ever understood the
full reasoning there.

> Alternately, encode this requirement into the methods themselves by leaving
this
> as two separate methods.

That's pretty much what was there before, but since 99% of the body is the same,
they just forwarded to a single _getDependencies() method. That would end up
with the same set of overlapping parameters, so wouldn't solve much.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:121: /// report of the changes
made relative to the previous lockfile.
On 2013/12/11 06:48:32, nweiz wrote:
> Explain that this is a temporary situation. Ideally, reference a bug for
getting
> detailed reports for get/serve/build as well.

Added a TODO below by the report code.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:125: Future<int>
getDependencies({List<String> useLatest, bool upgradeAll}) {
On 2013/12/11 06:48:32, nweiz wrote:
> I don't think I like [getDependencies] as the name here anymore, since most of
> the work it's doing is now upgrading packages and it's no longer specifically
> "the method that maps to 'pub get'." It's awkward that "get" means both
"install
> packages so they're accessible to the app" and "non-destructively get a value
> from an object", but this is running into that ambiguity and I think renaming
it
> would work better.

Yeah, I'm not crazy about it either. Changed to "acquireDependencies".

I have some half-formed thoughts to refactor this further but I didn't want to
go overboard on this patch.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:126: if (upgradeAll == null)
upgradeAll = false;
On 2013/12/11 06:48:32, nweiz wrote:
> I like using explicit defaults for boolean parameters.

That breaks forwarding. :(

upgradeAll is passed to resolveVersions which in turn passes it to
BacktrackingSolver. If we used an explicit default along the way, then passing
null along would cancel out the default.

Default params are horribly broken.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart (right):

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:105: _upgradeAll =
upgradeAll == true {
On 2013/12/11 06:48:32, nweiz wrote:
> Again, I think this would be much cleaner with an explicit default for
> [upgradeAll].

See previous comment.

> Also, keeping this as a flag as opposed to using it to modify [lockFile] means
> that you have to be sure that every time you access [lockFile] you remember to
> check [_upgradeAll] as well, which seems sub-optimal.

I was never crazy about mutating the lockfile in the solver. (I'm actually
tempted to make the class actually immutable.) With this patch, it's actually
important that the lock file not be changed because the report uses it to see
what changed from the previous lock file.

You're right that the solver has to check here (and does now), but that turned
out to just be a couple of places since accessing the lockfile goes through a
narrow set of methods already.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:149:
availableVersions[package.name] = versions;
On 2013/12/11 06:48:32, nweiz wrote:
> Factor this chunk out into _getAvailableVersions(PackageId package).

Done.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:153:
availableVersions, null, attemptedSolutions);
On 2013/12/11 06:48:32, nweiz wrote:
> This constructor is getting pretty unwieldy, especially with each invocation
> passing null to different parameters. At least make the parameters where one
or
> the other invocation passes null or {} into named parameters.

Split into two named constructors.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/solver/solve_report.dart (right):

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:16: class SolveReport {
On 2013/12/11 06:48:32, nweiz wrote:
> At first glance the fact that this is different than SolveResult is pretty
> confusing.

Added some more docs clarifying.

> 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.

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.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:29: int _pendingLines =
0;
On 2013/12/11 06:48:32, nweiz wrote:
> 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.

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.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:72: _output.clear();
On 2013/12/11 06:48:32, nweiz wrote:
> 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.

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.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:74: // Show the current
dependencies ordered by name.
On 2013/12/11 06:48:32, nweiz wrote:
> "current" -> "new"

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

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:114: if
(highlightOverride == null) highlightOverride = true;
On 2013/12/11 06:48:32, nweiz wrote:
> Blah blah explicit default.

Previous comment.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:140: _write(log.cyan("*
"));
On 2013/12/11 06:48:32, nweiz wrote:
> For colorblind folks like myself, it might be nice to have a comment somewhere
> explaining when you chose to use magenta vs cyan.

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

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:153: // If the package
was upgraded, show what it was upgraded from.
On 2013/12/11 06:48:32, nweiz wrote:
> This comment should be on the next block.

Done.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:211: // Unknown source,
so just do a literal comparison.
On 2013/12/11 06:48:32, nweiz wrote:
> 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.

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.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:242: /// Doesn't actually
immediately write. Instead, it waits until output is
On 2013/12/11 06:48:32, nweiz wrote:
> "immediately write a newline".

Done.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:243: /// written on the
nextline. That way, trailing newlines aren't displayed.
On 2013/12/11 06:48:32, nweiz wrote:
> "next line"

Done.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/solver/version_solver.dart (right):

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/version_solver.dart:195: /// by [package]
On 2013/12/11 06:48:32, nweiz wrote:
> Extra newline

Done.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/test/t...
File sdk/lib/_internal/pub/test/test_pub.dart (right):

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/test_pub.dart:272: RunCommand(this.name, String
pattern)
On 2013/12/11 06:48:32, nweiz wrote:
> If you're going to take a pattern, take a Pattern. Taking a string and
> converting it to a RegExp implicitly is confusing.

Done.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/test/u...
File sdk/lib/_internal/pub/test/upgrade/report/describes_change_test.dart
(right):

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/test/u...
sdk/lib/_internal/pub/test/upgrade/report/describes_change_test.dart:57: .
source_changed 2\.0\.0 from path \.\./source_changed \(was 1\.0\.0\)
On 2013/12/11 06:48:32, nweiz wrote:
> These "../" paths will probably break on Windows.

Done.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/test/u...
sdk/lib/_internal/pub/test/upgrade/report/describes_change_test.dart:59: .
version_changed 2\.0\.0 \(was 1\.0\.0\)
On 2013/12/11 06:48:32, nweiz wrote:
> Why are the initial characters here "."s?

To allow any of the leading character sigils since there's another test
specifically for those.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/test/u...
sdk/lib/_internal/pub/test/upgrade/report/describes_change_test.dart:60: """,
multiLine: true));
On 2013/12/11 06:48:32, nweiz wrote:
> This massively-escaped regexp is pretty gross and unreadable. Maybe allow
> [output] to be a list of patterns, each of which is matched against an
> individual line?

I just spent a good chunk of time on that and I don't feel like it was worth
doing. Taking a list means that we have to decide how it handles too many or too
few lines of output. This RegExp actually handles that correctly as-is: there
are a couple more lines of output ("Downloading...", etc.) that it ignores. I
don't want to re-invent "$" and "^" in a new API.

The regex is gross, but it works, is flexible, and doesn't force me to spend a
bunch of time yak shaving in the tests. These tests actually take advantage of
the expressiveness of regexes.

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

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:126: if (upgradeAll == null)
upgradeAll = false;
On 2013/12/11 22:36:59, Bob Nystrom wrote:
> On 2013/12/11 06:48:32, nweiz wrote:
> > I like using explicit defaults for boolean parameters.
> 
> That breaks forwarding. :(
> 
> upgradeAll is passed to resolveVersions which in turn passes it to
> BacktrackingSolver. If we used an explicit default along the way, then passing
> null along would cancel out the default.
>
> Default params are horribly broken.

I don't think we should expect or support null for named boolean parameters. If
we're forwarding the parameter, I think we should just have the default listed
in each declaration that gets forwarded. It's very risky to change the default
for a boolean parameter anyway, since anyone not passing that parameter may just
be doing so as a shorthand for "param: default", so it's unlikely that the two
parameters will get out-of-sync.

Using an explicit default here, though, avoids weird constructions like "param
== true" and makes it easy for a reader of the code or documentation to see what
the default is.

If you don't add explicit defaults, please be sure to document the default
values everywhere they appear.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart (right):

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:362: } else if
(!_upgradeAll && lockFile.packages.containsKey(package.name)) {
If you're going to keep _upgradeAll, then all accesses to [lockFile.packages]
should go through [getLocked] to ensure that they respect the same logic.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/solver/solve_report.dart (right):

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:16: class SolveReport {
On 2013/12/11 22:36:59, Bob Nystrom wrote:
> On 2013/12/11 06:48:32, nweiz wrote:
> > At first glance the fact that this is different than SolveResult is pretty
> > confusing.
> 
> Added some more docs clarifying.
> 
> > 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.
> 
> 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.

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.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:29: int _pendingLines =
0;
On 2013/12/11 22:36:59, Bob Nystrom wrote:
> On 2013/12/11 06:48:32, nweiz wrote:
> > 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.
> 
> 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.

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.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:74: // Show the current
dependencies ordered by name.
On 2013/12/11 22:36:59, Bob Nystrom wrote:
> On 2013/12/11 06:48:32, nweiz wrote:
> > "current" -> "new"
> 
> I used "current" here because "new" could mean "dependencies that were just
> added" and this includes existing ones as well.

"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.

https://codereview.chromium.org/103453005/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/solver/version_solver.dart (right):

https://codereview.chromium.org/103453005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/solver/version_solver.dart:74:
SolveResult.success(this._sources, this._root, this._previousLockFile,
this.packages,
Line length

[Bob Nystrom, 2013-12-12 00:14:57]

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:126: if (upgradeAll == null)
upgradeAll = false;
On 2013/12/11 23:31:27, nweiz wrote:
> On 2013/12/11 22:36:59, Bob Nystrom wrote:
> > On 2013/12/11 06:48:32, nweiz wrote:
> > > I like using explicit defaults for boolean parameters.
> > 
> > That breaks forwarding. :(
> > 
> > upgradeAll is passed to resolveVersions which in turn passes it to
> > BacktrackingSolver. If we used an explicit default along the way, then
passing
> > null along would cancel out the default.
> >
> > Default params are horribly broken.
> 
> I don't think we should expect or support null for named boolean parameters.
If
> we're forwarding the parameter, I think we should just have the default listed
> in each declaration that gets forwarded. It's very risky to change the default
> for a boolean parameter anyway, since anyone not passing that parameter may
just
> be doing so as a shorthand for "param: default", so it's unlikely that the two
> parameters will get out-of-sync.
> 
> Using an explicit default here, though, avoids weird constructions like "param
> == true" and makes it easy for a reader of the code or documentation to see
what
> the default is.

OK, done.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart (right):

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart:362: } else if
(!_upgradeAll && lockFile.packages.containsKey(package.name)) {
On 2013/12/11 23:31:27, nweiz wrote:
> If you're going to keep _upgradeAll, then all accesses to [lockFile.packages]
> should go through [getLocked] to ensure that they respect the same logic.

Done.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/solver/solve_report.dart (right):

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:16: class SolveReport {
On 2013/12/11 23:31:27, nweiz wrote:
> On 2013/12/11 22:36:59, Bob Nystrom wrote:
> > On 2013/12/11 06:48:32, nweiz wrote:
> > > At first glance the fact that this is different than SolveResult is pretty
> > > confusing.
> > 
> > Added some more docs clarifying.
> > 
> > > 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.
> > 
> > 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.
> 
> 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.

Done.

https://codereview.chromium.org/103453005/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:74: // Show the current
dependencies ordered by name.
On 2013/12/11 23:31:27, nweiz wrote:
> On 2013/12/11 22:36:59, Bob Nystrom wrote:
> > On 2013/12/11 06:48:32, nweiz wrote:
> > > "current" -> "new"
> > 
> > I used "current" here because "new" could mean "dependencies that were just
> > added" and this includes existing ones as well.
> 
> "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.

Done.

https://codereview.chromium.org/103453005/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/solver/version_solver.dart (right):

https://codereview.chromium.org/103453005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/solver/version_solver.dart:74:
SolveResult.success(this._sources, this._root, this._previousLockFile,
this.packages,
On 2013/12/11 23:31:27, nweiz wrote:
> Line length

Done.

[nweiz, 2013-12-12 21:20:10]

lgtm

https://codereview.chromium.org/103453005/diff/40001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/solver/solve_report.dart (right):

https://codereview.chromium.org/103453005/diff/40001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/solver/solve_report.dart:14: int
show(SourceRegistry sources, Package root, LockFile previousLockFile,
Add a doc comment (maybe just combine the existing comments for _SolveReport and
show).

[Bob Nystrom, 2013-12-13 00:54:26]

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