Throw ArgumentError if packageRoot is null.

Throw ArgumentError if packageRoot is null.

R=johnniwinther@google.com

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

[ahe, 2015-01-07 12:37:02]

ahe@google.com changed reviewers:
+ johnniwinther@google.com

[Johnni Winther, 2015-01-08 08:16:15]

lgtm

https://codereview.chromium.org/838773002/diff/1/dart/pkg/compiler/samples/js...
File dart/pkg/compiler/samples/jsonify/jsonify.dart (right):

https://codereview.chromium.org/838773002/diff/1/dart/pkg/compiler/samples/js...
dart/pkg/compiler/samples/jsonify/jsonify.dart:50:
handler.provider.cwd.resolve(Platform.packageRoot);
You need to ensure that the URI path ends with a /

[Platform.packageRoot] is _not_ the resolved base Uri but just the user-provided
string.

[ahe, 2015-01-08 08:34:44]

Thank you, Johnni! Please let me know if you think I'm being stupid for not
adding trailing slashes.

https://codereview.chromium.org/838773002/diff/1/dart/pkg/compiler/samples/js...
File dart/pkg/compiler/samples/jsonify/jsonify.dart (right):

https://codereview.chromium.org/838773002/diff/1/dart/pkg/compiler/samples/js...
dart/pkg/compiler/samples/jsonify/jsonify.dart:50:
handler.provider.cwd.resolve(Platform.packageRoot);
On 2015/01/08 08:16:15, Johnni Winther wrote:
> You need to ensure that the URI path ends with a /
> 
> [Platform.packageRoot] is _not_ the resolved base Uri but just the
user-provided
> string.

This isn't a super user friendly tool. For example, on line 43, an error will be
thrown if [arguments] is empty. I would rather not clutter this code with
handling of exceptional cases. Also, it isn't necessary, this tool doesn't read
packages.

In addition, it is possible to provide a usable package root which doesn't end
with a slash. It probably makes sense for dart2js (a directly user facing tool)
to complain about absence of a trailing slash, but I'm not convinced it is the
right trade-off for more low-level tools.

I'm going to submit this as-is, but please let me know if you disagree with my
analysis. I can create a follow-up CL.

[ahe, 2015-01-08 08:36:36]

Committed patchset #1 (id:1) manually as 42677 (presubmit successful).

[Johnni Winther, 2015-01-08 09:40:42]

https://codereview.chromium.org/838773002/diff/1/dart/pkg/compiler/samples/js...
File dart/pkg/compiler/samples/jsonify/jsonify.dart (right):

https://codereview.chromium.org/838773002/diff/1/dart/pkg/compiler/samples/js...
dart/pkg/compiler/samples/jsonify/jsonify.dart:50:
handler.provider.cwd.resolve(Platform.packageRoot);
On 2015/01/08 08:34:43, ahe wrote:
> On 2015/01/08 08:16:15, Johnni Winther wrote:
> > You need to ensure that the URI path ends with a /
> > 
> > [Platform.packageRoot] is _not_ the resolved base Uri but just the
> user-provided
> > string.
> 
> This isn't a super user friendly tool. For example, on line 43, an error will
be
> thrown if [arguments] is empty. I would rather not clutter this code with
> handling of exceptional cases. Also, it isn't necessary, this tool doesn't
read
> packages.
> 
> In addition, it is possible to provide a usable package root which doesn't end
> with a slash. It probably makes sense for dart2js (a directly user facing
tool)
> to complain about absence of a trailing slash, but I'm not convinced it is the
> right trade-off for more low-level tools.
> 
> I'm going to submit this as-is, but please let me know if you disagree with my
> analysis. I can create a follow-up CL.

The problem is really with Platform.packageRoot which should have been a fully
resolved URI.

[ahe, 2015-01-08 09:44:02]

On 2015/01/08 09:40:42, Johnni Winther wrote:
> The problem is really with Platform.packageRoot which should have been a fully
> resolved URI.

I agree. Perhaps we should add "Platform.resolvedPackageRoot".