Code coverage, something is working now.

Code coverage, something is working now.
For now only 'statement' level coverage.

Review this, please.
Any comments about Dart code style, using libraries, etc will be helpful!

There are some Dart VM changes, which I'm _not_ going to commit.
I'm waiting for https://codereview.chromium.org/16368002/
So, ignore them.


R=pquitslund@google.com
BUG=

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

[pquitslund, 2013-06-17 20:15:58]

LGTM.

My comments are pretty superficial.  I bet Bob will have something more
substantial to say! :)

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
File pkg/analyzer_experimental/bin/coverage.dart (right):

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
pkg/analyzer_experimental/bin/coverage.dart:83:
buffer.write('${_argParser.getUsage()}\n\n');
Cascade here?

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
File
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart
(right):

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart:84:
String content = rewritePathContent(path);
String -> var

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart:148:
request.listen((List<int> data) {
I'm guessing Bob will ask you to remove type annotations here (and just above
too).

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart:153:
int bitIndex = id % 8;
More possibly unnecessary/unwanted type annotations.

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
File
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_lib.dart
(right):

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_lib.dart:9:
import 'dart:typed_data';
I would pick one style of string literal ' vs " and stick with it consistently. 
FWIW: it seems that folks in general are trending towards '.

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_lib.dart:21:
HttpClient httpClient = new HttpClient();
var httpClient

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
File pkg/analyzer_experimental/lib/src/services/runtime/coverage/models.dart
(right):

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/models.dart:8:
import "dart:collection" show SplayTreeMap;
Again:  "->'

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
File pkg/analyzer_experimental/lib/src/services/runtime/coverage/utils.dart
(right):

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/utils.dart:9: bool
wasSR = false;
bool -> var

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/utils.dart:36: int
min = 0;
int -> var

[scheglov, 2013-06-17 20:50:59]

Thanks!

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
File pkg/analyzer_experimental/bin/coverage.dart (right):

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
pkg/analyzer_experimental/bin/coverage.dart:83:
buffer.write('${_argParser.getUsage()}\n\n');
On 2013/06/17 20:15:58, pquitslund wrote:
> Cascade here?

Done.

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
File
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart
(right):

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart:84:
String content = rewritePathContent(path);
On 2013/06/17 20:15:58, pquitslund wrote:
> String -> var

Done.

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart:148:
request.listen((List<int> data) {
On 2013/06/17 20:15:58, pquitslund wrote:
> I'm guessing Bob will ask you to remove type annotations here (and just above
> too).

Well, may be.
Unfortunately we don't have type propagation for closure arguments in the new
analyzer yet.
So, I will lose code completion.

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart:153:
int bitIndex = id % 8;
On 2013/06/17 20:15:58, pquitslund wrote:
> More possibly unnecessary/unwanted type annotations.

Done.

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
File
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_lib.dart
(right):

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_lib.dart:9:
import 'dart:typed_data';
On 2013/06/17 20:15:58, pquitslund wrote:
> I would pick one style of string literal ' vs " and stick with it
consistently. 
> FWIW: it seems that folks in general are trending towards '.

Done.

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_lib.dart:21:
HttpClient httpClient = new HttpClient();
On 2013/06/17 20:15:58, pquitslund wrote:
> var httpClient

Done.

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
File pkg/analyzer_experimental/lib/src/services/runtime/coverage/models.dart
(right):

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/models.dart:8:
import "dart:collection" show SplayTreeMap;
On 2013/06/17 20:15:58, pquitslund wrote:
> Again:  "->'

Done.

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
File pkg/analyzer_experimental/lib/src/services/runtime/coverage/utils.dart
(right):

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/utils.dart:9: bool
wasSR = false;
On 2013/06/17 20:15:58, pquitslund wrote:
> bool -> var

Done.

https://chromiumcodereview.appspot.com/16964008/diff/1/pkg/analyzer_experimen...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/utils.dart:36: int
min = 0;
On 2013/06/17 20:15:58, pquitslund wrote:
> int -> var

Done.

[Bob Nystrom, 2013-06-17 21:30:11]

I just skimmed it, but here's a few comments.

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/bin...
File pkg/analyzer_experimental/bin/coverage.dart (right):

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/bin...
pkg/analyzer_experimental/bin/coverage.dart:9: import "package:args/args.dart"
show ArgParser, ArgResults;
Is there anything you were specifically trying to not import here? Otherwise,
normal style is to not use "show".

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/bin...
pkg/analyzer_experimental/bin/coverage.dart:14: //
/Users/scheglov/Source/Dart/dart/pkg/analyzer_experimental/test/generated/all_test.dart
Remove this?

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/bin...
pkg/analyzer_experimental/bin/coverage.dart:75: ..addOption('port', help: 'The
port to run server on, if 0 select any.', defaultsTo: '0');
Long line. For Dart, we stick to 80 columns.

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/bin...
pkg/analyzer_experimental/bin/coverage.dart:84: print(buffer.toString());
Instead of building a buffer, I'd probably just print each line.

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/lib...
File
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart
(right):

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/lib...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart:19:
import 'package:analyzer_experimental/src/generated/engine.dart' show
RecordingErrorListener;
I'd remove these "show" clauses.

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/lib...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart:33:
.then((int port) {
Most people don't type annotate lambdas. It's usually obvious from the context.

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/lib...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart:39:
Process.start(dartExecutable, targetArgs).then((Process process) {
return Process.start...

If you create a future inside a .then() callback, you almost always want to
return it. That makes sure errors get piped through.

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/lib...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart:79:
var path = basePath + '/' + request.uri.path;
Use pathos.join for this.

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/lib...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart:80:
path = pathos.normalize(path);
If you're using pathos to work with URLs, you should create a builder
specifically for that. Otherwise, on Windows, it will think you're working with
windows file paths. You can do:

var builder = new pathos.Builder(style: pathos.url)

And then call the same methods on builder that you do on pathos.

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/lib...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart:83:
{
Is there a reason for this block?

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/lib...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart:95:
file.exists().then((bool found) {
.then() returns a future, and ideally you would do something to keep track of
that future. If an error is thrown from within it, it will exit the VM if
nothing handles it.

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/lib...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart:148:
request.listen((List<int> data) {
On 2013/06/17 20:15:58, pquitslund wrote:
> I'm guessing Bob will ask you to remove type annotations here (and just above
> too).

Yes!

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/lib...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart:164:
new File(outPath).writeAsString(sb.toString());
writeAsStringSync() or handle the returned future.

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/lib...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart:190:
if (path.contains('/packages/analyzer_experimental/')) {
return path.contains(...)

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/lib...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart:215:
'class __CCC extends __cc_ut.Configuration {'
Multiline string?

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
File
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_lib.dart
(right):

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_lib.dart:11:
const int PORT = 0; // replaced during rewrite
For constants, don't bother type annotating. It's obvious from the initializer.

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_lib.dart:12:
final Uint8List _executedIds = new Uint8List(1024 * 64);
Ditto for final variables. :)

Also, document those magic numbers.

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_lib.dart:22:
httpClient.post('127.0.0.1', PORT, '/statistics').then((HttpClientRequest
request) {
Return the result of calling post.

If you have a function that internally uses futures it should almost always
return a future. Otherwise, the calling code has no way of handling errors or
even telling when the operation is complete.

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
File pkg/analyzer_experimental/lib/src/services/runtime/coverage/models.dart
(right):

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/models.dart:18:
final List<NodeInfo> nodeStack = new List<NodeInfo>();
final nodeStack = <NodeInfo>[];

Ditto for other fields.

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/models.dart:43: void
print(StringSink sink, Set<int> executedIds) {
Calling a method "print" is probably asking for trouble. If you just do
"print()" inside another method in this class, it will call the top-level
print() function and not this one since lexical scope takes priority over
members in Dart.

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/models.dart:61:
final List<NodeInfo> children = <NodeInfo> [];
No space between > and [

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/models.dart:81: if
(children.isNotEmpty) {
Today I learned Iterable has isNotEmpty! :)

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/models.dart:86: });
Clever! You could also try:

children.fold(null, (prev, child) {
  if (prev != null) sink.writeln(',');
  child.print(sink, executedIds, '$prefix    ');
  return child;
});

That shouldn't have the overhead of creating the map.

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/models.dart:94:
idToRange.forEach(rangePrinter.handle);
Nice.

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/models.dart:154: ///
Containts information about the single unit of the application.
Containts -> Contains.

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/models.dart:158:
UnitInfo(AppInfo appInfo, String path, String content) : super(appInfo, null,
'unit', path) {
Move ": super..." to the next line.

[scheglov, 2013-06-17 22:30:56]

Thank you for comments!

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
File
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_lib.dart
(right):

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_lib.dart:11:
const int PORT = 0; // replaced during rewrite
On 2013/06/17 21:30:12, Bob Nystrom wrote:
> For constants, don't bother type annotating. It's obvious from the
initializer.

Done.

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_lib.dart:12:
final Uint8List _executedIds = new Uint8List(1024 * 64);
On 2013/06/17 21:30:12, Bob Nystrom wrote:
> Ditto for final variables. :)
> 
> Also, document those magic numbers.

Done.

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_lib.dart:22:
httpClient.post('127.0.0.1', PORT, '/statistics').then((HttpClientRequest
request) {
On 2013/06/17 21:30:12, Bob Nystrom wrote:
> Return the result of calling post.
> 
> If you have a function that internally uses futures it should almost always
> return a future. Otherwise, the calling code has no way of handling errors or
> even telling when the operation is complete.

Done.

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_lib.dart:24:
request.close();
Should I also return result of request.close() here?

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
File pkg/analyzer_experimental/lib/src/services/runtime/coverage/models.dart
(right):

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/models.dart:18:
final List<NodeInfo> nodeStack = new List<NodeInfo>();
On 2013/06/17 21:30:12, Bob Nystrom wrote:
> final nodeStack = <NodeInfo>[];
> 
> Ditto for other fields.

For all fields or for final fields with initializers?

Actually 'nextId' also has initializer.
But 'currentNode' is assigned 'null', so it is not obvious which type it is.

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/models.dart:43: void
print(StringSink sink, Set<int> executedIds) {
On 2013/06/17 21:30:12, Bob Nystrom wrote:
> Calling a method "print" is probably asking for trouble. If you just do
> "print()" inside another method in this class, it will call the top-level
> print() function and not this one since lexical scope takes priority over
> members in Dart.

Ah... yes.
Renamed to 'write'.

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/models.dart:61:
final List<NodeInfo> children = <NodeInfo> [];
On 2013/06/17 21:30:12, Bob Nystrom wrote:
> No space between > and [

Done.

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/models.dart:83:
children.asMap().forEach((int i, NodeInfo child) {
Is it recommended to have type annotations for closure parameters or skip them?

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/models.dart:86: });
On 2013/06/17 21:30:12, Bob Nystrom wrote:
> Clever! You could also try:
> 
> children.fold(null, (prev, child) {
>   if (prev != null) sink.writeln(',');
>   child.print(sink, executedIds, '$prefix    ');
>   return child;
> });
> 
> That shouldn't have the overhead of creating the map.

  More lines of code - I have to add "return" :-(
  Or use cascade trick:
return child..print(sink, executedIds, '$prefix    ');

  May be worth to write some helper.
  As I understand, there almost for sure was discussion to add forEach with
index into library and if it is not there, then there are no chance to get it.

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/models.dart:154: ///
Containts information about the single unit of the application.
On 2013/06/17 21:30:12, Bob Nystrom wrote:
> Containts -> Contains.

Done.

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/models.dart:158:
UnitInfo(AppInfo appInfo, String path, String content) : super(appInfo, null,
'unit', path) {
On 2013/06/17 21:30:12, Bob Nystrom wrote:
> Move ": super..." to the next line.

Done.

[Bob Nystrom, 2013-06-17 22:44:41]

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
File
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_lib.dart
(right):

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_lib.dart:24:
request.close();
On 2013/06/17 22:30:57, scheglov wrote:
> Should I also return result of request.close() here?

Yes, if it returns a future.

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
File pkg/analyzer_experimental/lib/src/services/runtime/coverage/models.dart
(right):

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/models.dart:18:
final List<NodeInfo> nodeStack = new List<NodeInfo>();
On 2013/06/17 22:30:57, scheglov wrote:
> On 2013/06/17 21:30:12, Bob Nystrom wrote:
> > final nodeStack = <NodeInfo>[];
> > 
> > Ditto for other fields.
> 
> For all fields or for final fields with initializers?

Just ones with initializers. If they aren't initialized, it's good to annotate
them.

> 
> Actually 'nextId' also has initializer.
> But 'currentNode' is assigned 'null', so it is not obvious which type it is.

Yup. Annotate this, but I wouldn't bother to initialize it to null since it does
that implicitly.

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/models.dart:83:
children.asMap().forEach((int i, NodeInfo child) {
On 2013/06/17 22:30:57, scheglov wrote:
> Is it recommended to have type annotations for closure parameters or skip
them?

The style guide says: "AVOID annotating types on function expressions."

I'm assuming at some point you'll add inference for them so we don't have to
annotate. :)

[scheglov, 2013-06-17 23:24:24]

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

[scheglov, 2013-06-18 06:11:03]

More comments addressed in new CL.
https://codereview.chromium.org/16885003

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/bin...
File pkg/analyzer_experimental/bin/coverage.dart (right):

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/bin...
pkg/analyzer_experimental/bin/coverage.dart:9: import "package:args/args.dart"
show ArgParser, ArgResults;
On 2013/06/17 21:30:11, Bob Nystrom wrote:
> Is there anything you were specifically trying to not import here? Otherwise,
> normal style is to not use "show".

Done.

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/bin...
pkg/analyzer_experimental/bin/coverage.dart:14: //
/Users/scheglov/Source/Dart/dart/pkg/analyzer_experimental/test/generated/all_test.dart
On 2013/06/17 21:30:11, Bob Nystrom wrote:
> Remove this?

Done.

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/bin...
pkg/analyzer_experimental/bin/coverage.dart:75: ..addOption('port', help: 'The
port to run server on, if 0 select any.', defaultsTo: '0');
On 2013/06/17 21:30:11, Bob Nystrom wrote:
> Long line. For Dart, we stick to 80 columns.

Done.

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/bin...
pkg/analyzer_experimental/bin/coverage.dart:84: print(buffer.toString());
On 2013/06/17 21:30:11, Bob Nystrom wrote:
> Instead of building a buffer, I'd probably just print each line.

Done.

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/lib...
File
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart
(right):

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/lib...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart:19:
import 'package:analyzer_experimental/src/generated/engine.dart' show
RecordingErrorListener;
On 2013/06/17 21:30:11, Bob Nystrom wrote:
> I'd remove these "show" clauses.

These libraries are huge.
I feel uncomfortable if I blindly import everything from them :-(

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/lib...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart:33:
.then((int port) {
On 2013/06/17 21:30:11, Bob Nystrom wrote:
> Most people don't type annotate lambdas. It's usually obvious from the
context.

Done.

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/lib...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart:39:
Process.start(dartExecutable, targetArgs).then((Process process) {
On 2013/06/17 21:30:11, Bob Nystrom wrote:
> return Process.start...
> 
> If you create a future inside a .then() callback, you almost always want to
> return it. That makes sure errors get piped through.

Done.

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/lib...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart:79:
var path = basePath + '/' + request.uri.path;
On 2013/06/17 21:30:11, Bob Nystrom wrote:
> Use pathos.join for this.

Done.

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/lib...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart:80:
path = pathos.normalize(path);
On 2013/06/17 21:30:11, Bob Nystrom wrote:
> If you're using pathos to work with URLs, you should create a builder
> specifically for that. Otherwise, on Windows, it will think you're working
with
> windows file paths. You can do:
> 
> var builder = new pathos.Builder(style: pathos.url)
> 
> And then call the same methods on builder that you do on pathos.

Done.

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/lib...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart:83:
{
On 2013/06/17 21:30:11, Bob Nystrom wrote:
> Is there a reason for this block?

Just to don't leak variable 'content' into outer scope.

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/lib...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart:95:
file.exists().then((bool found) {
On 2013/06/17 21:30:11, Bob Nystrom wrote:
> .then() returns a future, and ideally you would do something to keep track of
> that future. If an error is thrown from within it, it will exit the VM if
> nothing handles it.

I've added 'catchError'.

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/lib...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart:164:
new File(outPath).writeAsString(sb.toString());
On 2013/06/17 21:30:11, Bob Nystrom wrote:
> writeAsStringSync() or handle the returned future.

Actually I've found new File(outPath).openWrite() which is sync and I don't need
temporary StringBuffer.

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/lib...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart:190:
if (path.contains('/packages/analyzer_experimental/')) {
On 2013/06/17 21:30:11, Bob Nystrom wrote:
> return path.contains(...)

Done.

https://codereview.chromium.org/16964008/diff/1/pkg/analyzer_experimental/lib...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/coverage_impl.dart:215:
'class __CCC extends __cc_ut.Configuration {'
On 2013/06/17 21:30:11, Bob Nystrom wrote:
> Multiline string?

Done.

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
File pkg/analyzer_experimental/lib/src/services/runtime/coverage/models.dart
(right):

https://codereview.chromium.org/16964008/diff/8001/pkg/analyzer_experimental/...
pkg/analyzer_experimental/lib/src/services/runtime/coverage/models.dart:83:
children.asMap().forEach((int i, NodeInfo child) {
On 2013/06/17 22:44:41, Bob Nystrom wrote:
> On 2013/06/17 22:30:57, scheglov wrote:
> > Is it recommended to have type annotations for closure parameters or skip
> them?
> 
> The style guide says: "AVOID annotating types on function expressions."
> 
> I'm assuming at some point you'll add inference for them so we don't have to
> annotate. :)

Done.