appengine: throw AssetErrors directly (instead of via new Future.error

appengine: throw AssetErrors directly (instead of via new Future.error

Future closures wrap these correctly
Also: updated code review settings

R=kustermann@google.com

Committed: https://github.com/dart-lang/appengine/commit/4c262276686571a4f035c6010974a933d9585eb5

[kevmoo, 2014-11-10 21:56:52]

kevmoo@google.com changed reviewers:
+ kustermann@google.com, sgjesse@google.com

[kevmoo, 2014-11-10 21:57:56]

https://codereview.chromium.org/710093005/diff/1/lib/src/server/assets.dart
File lib/src/server/assets.dart (right):

https://codereview.chromium.org/710093005/diff/1/lib/src/server/assets.dart#n...
lib/src/server/assets.dart:50: print("Unable to connect to 'pub serve' for
'${request.uri}': $e");
Should we get rid of this print statement, too? Or move it to logging?

[kustermann, 2014-11-11 09:53:13]

Could you tell me what benefit you get by throwing instead of constructing an
error Future?

Is it that using package:stack_trace works better this way? Or what reason do
you have?

https://codereview.chromium.org/710093005/diff/1/codereview.settings
File codereview.settings (right):

https://codereview.chromium.org/710093005/diff/1/codereview.settings#newcode2
codereview.settings:2: VIEW_VC:
https://github.com/dart-lang/appengine_samples/commit/
This is not the appengine_samples repository!!!

https://codereview.chromium.org/710093005/diff/1/lib/src/server/assets.dart
File lib/src/server/assets.dart (right):

https://codereview.chromium.org/710093005/diff/1/lib/src/server/assets.dart#n...
lib/src/server/assets.dart:50: print("Unable to connect to 'pub serve' for
'${request.uri}': $e");
On 2014/11/10 21:57:56, kevmoo wrote:
> Should we get rid of this print statement, too? Or move it to logging?

"stdout" is currently our log for locally testing. Log messages here might not
be written with the logging service because the request is already done (see the
.pipe() above).

It's a nasty artifiact, that we can fix once we switch to the new way of doing
logging.

Please add a TODO here to use the logging service.

[kevmoo, 2014-11-11 15:15:52]

PTAL

Reason for this change: it's cleaner and more canonical.

It also ensures that the stack trace is captured.

https://codereview.chromium.org/710093005/diff/1/codereview.settings
File codereview.settings (right):

https://codereview.chromium.org/710093005/diff/1/codereview.settings#newcode2
codereview.settings:2: VIEW_VC:
https://github.com/dart-lang/appengine_samples/commit/
On 2014/11/11 09:53:13, kustermann wrote:
> This is not the appengine_samples repository!!!

oops. Good catch.

https://codereview.chromium.org/710093005/diff/1/lib/src/server/assets.dart
File lib/src/server/assets.dart (right):

https://codereview.chromium.org/710093005/diff/1/lib/src/server/assets.dart#n...
lib/src/server/assets.dart:50: print("Unable to connect to 'pub serve' for
'${request.uri}': $e");
On 2014/11/11 09:53:13, kustermann wrote:
> On 2014/11/10 21:57:56, kevmoo wrote:
> > Should we get rid of this print statement, too? Or move it to logging?
> 
> "stdout" is currently our log for locally testing. Log messages here might not
> be written with the logging service because the request is already done (see
the
> .pipe() above).
> 
> It's a nasty artifiact, that we can fix once we switch to the new way of doing
> logging.
> 
> Please add a TODO here to use the logging service.

Done.

[kustermann, 2014-11-11 17:05:51]

lgtm

[kevmoo, 2014-11-11 19:02:16]

Committed patchset #2 (id:20001) manually as
4c262276686571a4f035c6010974a933d9585eb5 (presubmit successful).