Restructured the set up for the compiling function. Also added room for list of additional summarie…

Restructured the set up for the compiling function. Also added room for list of additional summaries.

BUG=
R=jacobr@google.com, jmesserly@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/15f19aab3674915942b02f120f111c9c5c32815c

[priscillalee, 2016-08-03 23:34:46]

priscillalee@google.com changed reviewers:
+ jacobr@google.com, jmesserly@google.com

[Jacob, 2016-08-03 23:58:32]

https://codereview.chromium.org/2208243002/diff/1/pubspec.lock
File pubspec.lock (left):

https://codereview.chromium.org/2208243002/diff/1/pubspec.lock#oldcode208
pubspec.lock:208: shelf_packages_handler:
why did this go away? Make sure you are synced and have run
pub upgrade

https://codereview.chromium.org/2208243002/diff/1/web/main.dart
File web/main.dart (right):

https://codereview.chromium.org/2208243002/diff/1/web/main.dart#newcode52
web/main.dart:52: var args = ['compile'];
passing args and using command runner doesn't seem like it is doing anything
useful. am I missing something?

https://codereview.chromium.org/2208243002/diff/1/web/web_command.dart
File web/web_command.dart (right):

https://codereview.chromium.org/2208243002/diff/1/web/web_command.dart#newcode9
web/web_command.dart:9: import 'dart:async';
sort imports in alphabetical order.

https://codereview.chromium.org/2208243002/diff/1/web/web_command.dart#newcode56
web/web_command.dart:56: return requestSummaries;
Are we really getting anything from implementing Command? This seems like a
strange pattern given how the code is setup now.

https://codereview.chromium.org/2208243002/diff/1/web/web_command.dart#newcode76
web/web_command.dart:76: });
You should handle if a summary fails to load.

https://codereview.chromium.org/2208243002/diff/1/web/web_command.dart#newcod...
web/web_command.dart:117: "", "", ["file:///expression$number.dart"], (source)
=> "");
nit: put {} around number for clarity. 
e.g.
${number}

https://codereview.chromium.org/2208243002/diff/1/web/web_command.dart#newcod...
web/web_command.dart:117: "", "", ["file:///expression$number.dart"], (source)
=> "");
what is the fn (source) => "" doing?
shouldn't it be the string dart?

[priscillalee, 2016-08-04 16:01:56]

https://codereview.chromium.org/2208243002/diff/1/pubspec.lock
File pubspec.lock (left):

https://codereview.chromium.org/2208243002/diff/1/pubspec.lock#oldcode208
pubspec.lock:208: shelf_packages_handler:
On 2016/08/03 23:58:31, Jacob wrote:
> why did this go away? Make sure you are synced and have run
> pub upgrade

Done.

https://codereview.chromium.org/2208243002/diff/1/web/main.dart
File web/main.dart (right):

https://codereview.chromium.org/2208243002/diff/1/web/main.dart#newcode52
web/main.dart:52: var args = ['compile'];
On 2016/08/03 23:58:31, Jacob wrote:
> passing args and using command runner doesn't seem like it is doing anything
> useful. am I missing something?

I looked into this myself earlier because I was wondering the same thing. I
think something happens behind the scenes with ArgResults or ArgParsers, and I
wasn't able to remove the CommandRunner or the args after a couple tries. I'm
also not quite sure how this works. Do you think it's worth it to continue
investigating?

https://codereview.chromium.org/2208243002/diff/1/web/web_command.dart
File web/web_command.dart (right):

https://codereview.chromium.org/2208243002/diff/1/web/web_command.dart#newcode9
web/web_command.dart:9: import 'dart:async';
On 2016/08/03 23:58:32, Jacob wrote:
> sort imports in alphabetical order.

Done.

https://codereview.chromium.org/2208243002/diff/1/web/web_command.dart#newcode56
web/web_command.dart:56: return requestSummaries;
On 2016/08/03 23:58:31, Jacob wrote:
> Are we really getting anything from implementing Command? This seems like a
> strange pattern given how the code is setup now.

I agree. It does seem strange. I don't understand how the args or commands are
set up, but I'm mimicking the patterns of bin/dartdevc.dart and
lib/src/compiler/command.dart. I'm not sure how else to structure it.

https://codereview.chromium.org/2208243002/diff/1/web/web_command.dart#newcode76
web/web_command.dart:76: });
On 2016/08/03 23:58:31, Jacob wrote:
> You should handle if a summary fails to load.

I added status checks, but should I also throw exceptions if a summary fails to
load?

https://codereview.chromium.org/2208243002/diff/1/web/web_command.dart#newcod...
web/web_command.dart:117: "", "", ["file:///expression$number.dart"], (source)
=> "");
On 2016/08/03 23:58:32, Jacob wrote:
> nit: put {} around number for clarity. 
> e.g.
> ${number}

Done.

https://codereview.chromium.org/2208243002/diff/1/web/web_command.dart#newcod...
web/web_command.dart:117: "", "", ["file:///expression$number.dart"], (source)
=> "");
On 2016/08/03 23:58:32, Jacob wrote:
> what is the fn (source) => "" doing?
> shouldn't it be the string dart?

It's the libraryToModule function of a BuildUnit. I used to be able to give it
null, but when I started to work with additional summaries, it needed a dummy
function. I haven't come across a point when I've needed it to return a specific
String, so it currently has no use right now.

[Jacob, 2016-08-04 16:27:48]

https://codereview.chromium.org/2208243002/diff/1/web/main.dart
File web/main.dart (right):

https://codereview.chromium.org/2208243002/diff/1/web/main.dart#newcode52
web/main.dart:52: var args = ['compile'];
On 2016/08/04 16:01:56, priscillalee wrote:
> On 2016/08/03 23:58:31, Jacob wrote:
> > passing args and using command runner doesn't seem like it is doing anything
> > useful. am I missing something?
> 
> I looked into this myself earlier because I was wondering the same thing. I
> think something happens behind the scenes with ArgResults or ArgParsers, and I
> wasn't able to remove the CommandRunner or the args after a couple tries. I'm
> also not quite sure how this works. Do you think it's worth it to continue
> investigating?

Just add a TODO indicating just that so the next person to look at this code
knows it is a known issue but fixing may be harder than it seems.

https://codereview.chromium.org/2208243002/diff/1/web/web_command.dart
File web/web_command.dart (right):

https://codereview.chromium.org/2208243002/diff/1/web/web_command.dart#newcode76
web/web_command.dart:76: });
On 2016/08/04 16:01:56, priscillalee wrote:
> On 2016/08/03 23:58:31, Jacob wrote:
> > You should handle if a summary fails to load.
> 
> I added status checks, but should I also throw exceptions if a summary fails
to
> load?

Ideally you would report the error back to the caller so they can display an
appropriate message in the App's UI explaining that something went wrong.
Consider just having the callback the user passes in taking a second argument
that is an error message if compilation or summaries failed.

https://codereview.chromium.org/2208243002/diff/1/web/web_command.dart#newcod...
web/web_command.dart:117: "", "", ["file:///expression$number.dart"], (source)
=> "");
On 2016/08/03 23:58:32, Jacob wrote:
> nit: put {} around number for clarity. 
> e.g.
> ${number}

do that everywhere in the file where $number occurs.

https://codereview.chromium.org/2208243002/diff/20001/web/web_command.dart
File web/web_command.dart (right):

https://codereview.chromium.org/2208243002/diff/20001/web/web_command.dart#ne...
web/web_command.dart:75: if (response.readyState == HttpRequest.DONE &&
I don't think these status checks are needed. I believe the future will not
complete in the then clause if a failure case was hit. Instead you need to use
the onError handler of the Future.

[priscillalee, 2016-08-04 17:56:33]

https://codereview.chromium.org/2208243002/diff/1/web/main.dart
File web/main.dart (right):

https://codereview.chromium.org/2208243002/diff/1/web/main.dart#newcode52
web/main.dart:52: var args = ['compile'];
On 2016/08/04 16:27:48, Jacob wrote:
> On 2016/08/04 16:01:56, priscillalee wrote:
> > On 2016/08/03 23:58:31, Jacob wrote:
> > > passing args and using command runner doesn't seem like it is doing
anything
> > > useful. am I missing something?
> > 
> > I looked into this myself earlier because I was wondering the same thing. I
> > think something happens behind the scenes with ArgResults or ArgParsers, and
I
> > wasn't able to remove the CommandRunner or the args after a couple tries.
I'm
> > also not quite sure how this works. Do you think it's worth it to continue
> > investigating?
> 
> Just add a TODO indicating just that so the next person to look at this code
> knows it is a known issue but fixing may be harder than it seems.

Done.

https://codereview.chromium.org/2208243002/diff/1/web/web_command.dart
File web/web_command.dart (right):

https://codereview.chromium.org/2208243002/diff/1/web/web_command.dart#newcode76
web/web_command.dart:76: });
On 2016/08/04 16:27:48, Jacob wrote:
> On 2016/08/04 16:01:56, priscillalee wrote:
> > On 2016/08/03 23:58:31, Jacob wrote:
> > > You should handle if a summary fails to load.
> > 
> > I added status checks, but should I also throw exceptions if a summary fails
> to
> > load?
> 
> Ideally you would report the error back to the caller so they can display an
> appropriate message in the App's UI explaining that something went wrong.
> Consider just having the callback the user passes in taking a second argument
> that is an error message if compilation or summaries failed.

Done.

https://codereview.chromium.org/2208243002/diff/1/web/web_command.dart#newcod...
web/web_command.dart:117: "", "", ["file:///expression$number.dart"], (source)
=> "");
On 2016/08/04 16:27:48, Jacob wrote:
> On 2016/08/03 23:58:32, Jacob wrote:
> > nit: put {} around number for clarity. 
> > e.g.
> > ${number}
> 
> do that everywhere in the file where $number occurs.

Done.

https://codereview.chromium.org/2208243002/diff/20001/web/web_command.dart
File web/web_command.dart (right):

https://codereview.chromium.org/2208243002/diff/20001/web/web_command.dart#ne...
web/web_command.dart:75: if (response.readyState == HttpRequest.DONE &&
On 2016/08/04 16:27:48, Jacob wrote:
> I don't think these status checks are needed. I believe the future will not
> complete in the then clause if a failure case was hit. Instead you need to use
> the onError handler of the Future.

Done.

[Jacob, 2016-08-04 19:04:13]

lgtm

[Jennifer Messerly, 2016-08-09 17:32:06]

lgtm

[priscillalee, 2016-08-09 17:40:41]

Description was changed from

==========
Restructured the set up for the compiling function. Also added room for list of
additional summaries.

BUG=
==========

to

==========
Restructured the set up for the compiling function. Also added room for list of
additional summaries.

BUG=
R=jacobr@google.com, jmesserly@google.com

Committed:
https://github.com/dart-lang/dev_compiler/commit/15f19aab3674915942b02f120f11...
==========

[priscillalee, 2016-08-09 17:40:43]

Committed patchset #4 (id:60001) manually as
15f19aab3674915942b02f120f111c9c5c32815c (presubmit successful).

[priscillalee, 2016-08-10 01:12:59]

Patchset #5 (id:80001) has been deleted