Issue 22878002: Handle missing files more gracefully in pub serve.

Issue 22878002: Handle missing files more gracefully in pub serve. (Closed)

Created:
7 years, 4 months ago by Bob Nystrom

Modified:
7 years, 4 months ago

Reviewers:
nweiz

CC:
reviews_dartlang.org

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Visibility:
Public.

More Reviews

Description

Handle missing files more gracefully in pub serve. BUG= R=nweiz@google.com Committed: https://code.google.com/p/dart/source/detail?r=26036

Patch Set 2 : Revise. #

Total comments: 2

Created: 7 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+97 lines, -12 lines)			Patch
M	sdk/lib/_internal/pub/lib/src/command/serve.dart	View	1	2 chunks	+26 lines, -9 lines	0 comments	Download
M	sdk/lib/_internal/pub/lib/src/utils.dart	View	1	1 chunk	+40 lines, -0 lines	2 comments	Download
A +	sdk/lib/_internal/pub/test/serve/missing_asset_test.dart	View		1 chunk	+1 line, -1 line	0 comments	Download
M	sdk/lib/_internal/pub/test/serve/missing_file_test.dart	View	1	2 chunks	+30 lines, -2 lines	0 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

nweiz

A few suggestions, otherwise lgtm. https://codereview.chromium.org/22878002/diff/1/sdk/lib/_internal/pub/lib/src/command/serve.dart File sdk/lib/_internal/pub/lib/src/command/serve.dart (right): https://codereview.chromium.org/22878002/diff/1/sdk/lib/_internal/pub/lib/src/command/serve.dart#newcode104 sdk/lib/_internal/pub/lib/src/command/serve.dart:104: log.error("$_red${request.method}$_none ${request.uri} -> $error"); ...

7 years, 4 months ago (2013-08-12 20:26:31 UTC) #2

Bob Nystrom

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

7 years, 4 months ago (2013-08-12 21:32:36 UTC) #3

nweiz

https://codereview.chromium.org/22878002/diff/7001/sdk/lib/_internal/pub/lib/src/utils.dart File sdk/lib/_internal/pub/lib/src/utils.dart (right): https://codereview.chromium.org/22878002/diff/7001/sdk/lib/_internal/pub/lib/src/utils.dart#newcode194 sdk/lib/_internal/pub/lib/src/utils.dart:194: if (completer.isCompleted) controller.addError(error); I think you want to return ...

7 years, 4 months ago (2013-08-12 21:35:08 UTC) #4

Bob Nystrom

7 years, 4 months ago (2013-08-12 21:44:59 UTC) #5

Message was sent while issue was closed.

https://codereview.chromium.org/22878002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/command/serve.dart (right):

https://codereview.chromium.org/22878002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:104:
log.error("$_red${request.method}$_none ${request.uri} -> $error");
On 2013/08/12 20:26:31, nweiz wrote:
> long line

Done.

https://codereview.chromium.org/22878002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/utils.dart (right):

https://codereview.chromium.org/22878002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/utils.dart:185: var controller = new
StreamController();
On 2013/08/12 20:26:31, nweiz wrote:
> Make this sync? Since [stream] will already be async, adding an extra event
loop
> hop is probably unnecessary.

Good call. Done.

https://codereview.chromium.org/22878002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/utils.dart:193: if (!completer.isCompleted)
completer.completeError(error);
On 2013/08/12 20:26:31, nweiz wrote:
> If we get an error first, we should cancel our subscription to the stream.

Done.

https://codereview.chromium.org/22878002/diff/1/sdk/lib/_internal/pub/test/se...
File sdk/lib/_internal/pub/test/serve/missing_file_test.dart (right):

https://codereview.chromium.org/22878002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/missing_file_test.dart:38: // underlying file
does not exist.
On 2013/08/12 20:26:31, nweiz wrote:
> If you make the polling interval customizable, you could just set it to a year
> or something.

I was thinking the same thing. Updated the comment.

https://codereview.chromium.org/22878002/diff/7001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/utils.dart (right):

https://codereview.chromium.org/22878002/diff/7001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/utils.dart:194: if (completer.isCompleted)
controller.addError(error);
On 2013/08/12 21:35:08, nweiz wrote:
> I think you want to return here. Right now you'll be double-completing the
> completer if there's an error after the first result.

Oops, accidentally deleted that when I reordered the if and else cases. See:
https://codereview.chromium.org/22952002

Expand Messages | Collapse Messages