Include file path in JSON error when possible.

Include file path in JSON error when possible.

I also cleaned up a bunch of unused imports.

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

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

[nweiz, 2014-07-16 18:01:42]

It would be nice to have tests of the lockfile-validation behavior like we do
for pubspecs.

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

https://codereview.chromium.org/391363004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/lock_file.dart:57: if (packageEntries != null) {
Validate that `packageEntries` is a map.

https://codereview.chromium.org/391363004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/lock_file.dart:61: 'Package $name is missing a
version.', spec);
These errors originally included the package name as a means of helping the user
figure out where the error was. Since SpanFormatException does that better,
including the name in the error message seems redundant.

https://codereview.chromium.org/391363004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/lock_file.dart:76: fromLockFile: true);
Catch a FormatException here and convert it to a SpanFormatException associated
with `description`'s span.

https://codereview.chromium.org/391363004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/lock_file.dart:82: "Package name $name doesn't
match ${id.name}.", parsed);
`parsed` is the entire lockfile; that's not the span we want here. I think this
should be either `spec` or the span for `name`.

https://codereview.chromium.org/391363004/diff/1/sdk/lib/_internal/pub/test/s...
File sdk/lib/_internal/pub/test/serve/utils.dart (right):

https://codereview.chromium.org/391363004/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/utils.dart:347: return schedule(() {
Since this return value is clearly never used, I'd favor making the function
void over returning the future.

[Bob Nystrom, 2014-07-16 18:34:45]

> It would be nice to have tests of the lockfile-validation behavior like we do
for pubspecs.

lock_file_test already exists and does that. Added a couple more cases for the
new validation added here.

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

https://codereview.chromium.org/391363004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/lock_file.dart:57: if (packageEntries != null) {
On 2014/07/16 18:01:42, nweiz wrote:
> Validate that `packageEntries` is a map.

Done.

https://codereview.chromium.org/391363004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/lock_file.dart:61: 'Package $name is missing a
version.', spec);
On 2014/07/16 18:01:42, nweiz wrote:
> These errors originally included the package name as a means of helping the
user
> figure out where the error was. Since SpanFormatException does that better,
> including the name in the error message seems redundant.

I kind of like leaving in there because it reads well and means the user doesn't
have to hunt the package name down in the path, which may be unfamiliar to them
if it points into the cache.

https://codereview.chromium.org/391363004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/lock_file.dart:76: fromLockFile: true);
On 2014/07/16 18:01:42, nweiz wrote:
> Catch a FormatException here and convert it to a SpanFormatException
associated
> with `description`'s span.

Done.

https://codereview.chromium.org/391363004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/lock_file.dart:82: "Package name $name doesn't
match ${id.name}.", parsed);
On 2014/07/16 18:01:42, nweiz wrote:
> `parsed` is the entire lockfile; that's not the span we want here. I think
this
> should be either `spec` or the span for `name`.

Done.

https://codereview.chromium.org/391363004/diff/1/sdk/lib/_internal/pub/test/s...
File sdk/lib/_internal/pub/test/serve/utils.dart (right):

https://codereview.chromium.org/391363004/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/utils.dart:347: return schedule(() {
On 2014/07/16 18:01:42, nweiz wrote:
> Since this return value is clearly never used, I'd favor making the function
> void over returning the future.

Done.

[nweiz, 2014-07-17 07:11:19]

lgtm

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

https://codereview.chromium.org/391363004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/lock_file.dart:61: 'Package $name is missing a
version.', spec);
On 2014/07/16 18:34:44, Bob Nystrom wrote:
> On 2014/07/16 18:01:42, nweiz wrote:
> > These errors originally included the package name as a means of helping the
> user
> > figure out where the error was. Since SpanFormatException does that better,
> > including the name in the error message seems redundant.
> 
> I kind of like leaving in there because it reads well and means the user
doesn't
> have to hunt the package name down in the path, which may be unfamiliar to
them
> if it points into the cache.

The span will include the package name. I feel like it will read redundantly,
but DWYW.

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

https://codereview.chromium.org/391363004/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/lock_file.dart:81: } on FormatException catch(ex)
{
Nit: space after "catch".

[Bob Nystrom, 2014-07-17 22:14:36]

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

[Bob Nystrom, 2014-07-17 22:19:19]

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

https://codereview.chromium.org/391363004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/lock_file.dart:61: 'Package $name is missing a
version.', spec);
On 2014/07/17 07:11:19, nweiz wrote:
> On 2014/07/16 18:34:44, Bob Nystrom wrote:
> > On 2014/07/16 18:01:42, nweiz wrote:
> > > These errors originally included the package name as a means of helping
the
> > user
> > > figure out where the error was. Since SpanFormatException does that
better,
> > > including the name in the error message seems redundant.
> > 
> > I kind of like leaving in there because it reads well and means the user
> doesn't
> > have to hunt the package name down in the path, which may be unfamiliar to
> them
> > if it points into the cache.
> 
> The span will include the package name. I feel like it will read redundantly,
> but DWYW.

Acknowledged.

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

https://codereview.chromium.org/391363004/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/lock_file.dart:81: } on FormatException catch(ex)
{
On 2014/07/17 07:11:19, nweiz wrote:
> Nit: space after "catch".

Done.