Better progress reporting.

Better progress reporting.

BUG=https://code.google.com/p/dart/issues/detail?id=17481
R=nweiz@google.com

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

[nweiz, 2014-03-14 23:30:29]

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

https://codereview.chromium.org/200573007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:39: /// The currently-running progress
bar or `null` if there is none.
"progress bar" -> "progress indicator"

https://codereview.chromium.org/200573007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:236: _stopProgress() {
This doesn't seem worth having in a separate function anymore as opposed to just
putting it directly in [whenComplete]. You could even get rid of [_progress].

https://codereview.chromium.org/200573007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:237: if (_progress == null) return;
There's no way for [_progress] to be null here, is there?

https://codereview.chromium.org/200573007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:405: /// A live-updating progress bar for
long-running log entries.
"bar" -> "indicator"

https://codereview.chromium.org/200573007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:406: class _Progress {
Nit: This is independent enough from the rest of this file that I'd probably put
it in its own library. Maybe log/progress.dart?

https://codereview.chromium.org/200573007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:427: stdout.write("$_message... ");
Won't writing directly to stdout interact poorly with --verbose? Maybe
[progress] should avoid invoking [_Progress] in verbose mode?

https://codereview.chromium.org/200573007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:431: void stop() {
Document this.

https://codereview.chromium.org/200573007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:436: stdout.writeln(gray(_time));
No real reason to use [gray] here, since we're not writing to a TTY anyway.

https://codereview.chromium.org/200573007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:447: _transcript.add(new
Entry(Level.MESSAGE, ["$_message... ${_time}"]));
If you move [_Progress] into it's library, you could move this into [progress].
That actually seems like it would be good anyway, since we probably want to add
this in --verbose mode and with JSON formatting.

https://codereview.chromium.org/200573007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:461: return time + "$s.${ms}s)";
Add a TODO to put a general [formatDuration] function in utils or quiver or
something.

[Bob Nystrom, 2014-03-15 00:03:51]

Thanks!

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

https://codereview.chromium.org/200573007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:39: /// The currently-running progress
bar or `null` if there is none.
On 2014/03/14 23:30:30, nweiz wrote:
> "progress bar" -> "progress indicator"

Done.

https://codereview.chromium.org/200573007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:236: _stopProgress() {
On 2014/03/14 23:30:30, nweiz wrote:
> This doesn't seem worth having in a separate function anymore as opposed to
just
> putting it directly in [whenComplete]. You could even get rid of [_progress].

It's called by _printToStream too. That way if anything else is logged while
progress is going on, it gracefully cancels out the progress.

https://codereview.chromium.org/200573007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:237: if (_progress == null) return;
On 2014/03/14 23:30:30, nweiz wrote:
> There's no way for [_progress] to be null here, is there?

Yeah, see above. It gets called before all non-progress logs.

https://codereview.chromium.org/200573007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:405: /// A live-updating progress bar for
long-running log entries.
On 2014/03/14 23:30:30, nweiz wrote:
> "bar" -> "indicator"

Done.

https://codereview.chromium.org/200573007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:406: class _Progress {
On 2014/03/14 23:30:30, nweiz wrote:
> Nit: This is independent enough from the rest of this file that I'd probably
put
> it in its own library. Maybe log/progress.dart?

Done.

https://codereview.chromium.org/200573007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:427: stdout.write("$_message... ");
On 2014/03/14 23:30:30, nweiz wrote:
> Won't writing directly to stdout interact poorly with --verbose?

It works out OK. It doesn't have the header printed, but it doesn't look bad.

> Maybe [progress] should avoid invoking [_Progress] in verbose mode?

I'm doing this deliberately so that the transcript contains an entry for the
progress and tracks its time.

https://codereview.chromium.org/200573007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:431: void stop() {
On 2014/03/14 23:30:30, nweiz wrote:
> Document this.

Done.

https://codereview.chromium.org/200573007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:436: stdout.writeln(gray(_time));
On 2014/03/14 23:30:30, nweiz wrote:
> No real reason to use [gray] here, since we're not writing to a TTY anyway.

Done.

https://codereview.chromium.org/200573007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:447: _transcript.add(new
Entry(Level.MESSAGE, ["$_message... ${_time}"]));
On 2014/03/14 23:30:30, nweiz wrote:
> If you move [_Progress] into it's library, you could move this into
[progress].
> That actually seems like it would be good anyway, since we probably want to
add
> this in --verbose mode and with JSON formatting.

Done.

https://codereview.chromium.org/200573007/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:461: return time + "$s.${ms}s)";
On 2014/03/14 23:30:30, nweiz wrote:
> Add a TODO to put a general [formatDuration] function in utils or quiver or
> something.

Done.

[nweiz, 2014-03-15 00:19:13]

lgtm

[Bob Nystrom, 2014-03-15 00:23:41]

Committed patchset #2 manually as r33728 (presubmit successful).