fixes #610, incorrect help output

fixes #610, incorrect help output

R=nweiz@google.com

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

[Jennifer Messerly, 2016-08-12 18:26:58]

jmesserly@google.com changed reviewers:
+ nweiz@google.com, vsm@google.com

[nweiz, 2016-08-12 20:30:57]

https://codereview.chromium.org/2244703003/diff/40001/bin/dartdevc.dart
File bin/dartdevc.dart (right):

https://codereview.chromium.org/2244703003/diff/40001/bin/dartdevc.dart#newco...
bin/dartdevc.dart:51: exitCode = _runCommand(args);
I'd personally be more thorough here and remove all the vestiges of the command
structure—make the code look like how you'd write it if you were writing a
non-command-based executable from the start. Probably just make CompileCommand a
single top-level function rather than a class, since it doesn't override
anything anymore.

https://codereview.chromium.org/2244703003/diff/40001/bin/dartdevc.dart#newco...
bin/dartdevc.dart:58: // it at some point.
Is it really important to be backwards compatible here? I doubt you have any
external users who are relying strongly on existing behavior, and IIRC most of
the time the command structure existed it didn't even work when invoked from the
SDK.

https://codereview.chromium.org/2244703003/diff/40001/bin/dartdevc.dart#newco...
bin/dartdevc.dart:79: return new Future.value(new WorkResponse()
Nit, but if a function doesn't do any async work but has to return a future to
fit an interface, I usually annotate it "async" rather than using "new
Future.value". That way you can refactor the function internals all you want
without messing with the semantics.

https://codereview.chromium.org/2244703003/diff/40001/lib/src/analyzer/contex...
File lib/src/analyzer/context.dart (right):

https://codereview.chromium.org/2244703003/diff/40001/lib/src/analyzer/contex...
lib/src/analyzer/context.dart:77: static ArgParser addArguments(ArgParser
parser) {
I know this is outside the scope of the changes being made in this CL, but it
really bugs me that this takes a mutable argument, mutates it, and just returns
the argument as-is. What's the point? Why not just do the mutation, like all
other mutating methods do?

https://codereview.chromium.org/2244703003/diff/40001/lib/src/compiler/comman...
File lib/src/compiler/command.dart (right):

https://codereview.chromium.org/2244703003/diff/40001/lib/src/compiler/comman...
lib/src/compiler/command.dart:24: final ArgParser _argParser = new ArgParser();
Nit: this doesn't need to be type-annotated.

https://codereview.chromium.org/2244703003/diff/40001/lib/src/compiler/comman...
lib/src/compiler/command.dart:76: _print("```");
Make this a multi-line string rather than calling _print over and over. If a
MessageHandler only supports a single line at a time, add a helper method to
split a string and call _print on the results rather than doing it manually.

https://codereview.chromium.org/2244703003/diff/40001/lib/src/compiler/compil...
File lib/src/compiler/compiler.dart (right):

https://codereview.chromium.org/2244703003/diff/40001/lib/src/compiler/compil...
lib/src/compiler/compiler.dart:251: help: 'file extension for Dart summary
files', defaultsTo: 'sum', hide: true)
Long line.

https://codereview.chromium.org/2244703003/diff/40001/lib/src/compiler/compil...
lib/src/compiler/compiler.dart:282: help: 'Hoist types from class signatures',
defaultsTo: false, hide: true)
Long line.

[Jennifer Messerly, 2016-08-12 21:08:01]

PTAL

https://codereview.chromium.org/2244703003/diff/40001/bin/dartdevc.dart
File bin/dartdevc.dart (right):

https://codereview.chromium.org/2244703003/diff/40001/bin/dartdevc.dart#newco...
bin/dartdevc.dart:51: exitCode = _runCommand(args);
On 2016/08/12 20:30:57, nweiz wrote:
> I'd personally be more thorough here and remove all the vestiges of the
command
> structure—make the code look like how you'd write it if you were writing a
> non-command-based executable from the start. Probably just make CompileCommand
a
> single top-level function rather than a class, since it doesn't override
> anything anymore.

yeah, it was kind of a convenient place to collect state, that's why I left it
(the whole objects-are-closures correspondence). but I went ahead and refactored
it away.

https://codereview.chromium.org/2244703003/diff/40001/bin/dartdevc.dart#newco...
bin/dartdevc.dart:58: // it at some point.
On 2016/08/12 20:30:57, nweiz wrote:
> Is it really important to be backwards compatible here? I doubt you have any
> external users who are relying strongly on existing behavior, and IIRC most of
> the time the command structure existed it didn't even work when invoked from
the
> SDK.

our internal build rules still pass it

https://codereview.chromium.org/2244703003/diff/40001/lib/src/analyzer/contex...
File lib/src/analyzer/context.dart (right):

https://codereview.chromium.org/2244703003/diff/40001/lib/src/analyzer/contex...
lib/src/analyzer/context.dart:77: static ArgParser addArguments(ArgParser
parser) {
On 2016/08/12 20:30:57, nweiz wrote:
> I know this is outside the scope of the changes being made in this CL, but it
> really bugs me that this takes a mutable argument, mutates it, and just
returns
> the argument as-is. What's the point? Why not just do the mutation, like all
> other mutating methods do?

allows it to be used in cascade fashion, as typical for builders. I went ahead
and made it a void return

https://codereview.chromium.org/2244703003/diff/40001/lib/src/compiler/comman...
File lib/src/compiler/command.dart (right):

https://codereview.chromium.org/2244703003/diff/40001/lib/src/compiler/comman...
lib/src/compiler/command.dart:24: final ArgParser _argParser = new ArgParser();
On 2016/08/12 20:30:57, nweiz wrote:
> Nit: this doesn't need to be type-annotated.

true. Although it unfortunately needs the type annotation after removing the
class & having addArguments methods return "void". (see updated version of this
file)

https://codereview.chromium.org/2244703003/diff/40001/lib/src/compiler/comman...
lib/src/compiler/command.dart:76: _print("```");
On 2016/08/12 20:30:57, nweiz wrote:
> Make this a multi-line string rather than calling _print over and over. If a
> MessageHandler only supports a single line at a time, add a helper method to
> split a string and call _print on the results rather than doing it manually.

Done. As a general rule I try and not fix-all-the-things when doing a refactor.
But that said I'm happy to fix this case as it's easy enough and a lot nicer
looking.

https://codereview.chromium.org/2244703003/diff/40001/lib/src/compiler/compil...
File lib/src/compiler/compiler.dart (right):

https://codereview.chromium.org/2244703003/diff/40001/lib/src/compiler/compil...
lib/src/compiler/compiler.dart:251: help: 'file extension for Dart summary
files', defaultsTo: 'sum', hide: true)
On 2016/08/12 20:30:57, nweiz wrote:
> Long line.

Done.

https://codereview.chromium.org/2244703003/diff/40001/lib/src/compiler/compil...
lib/src/compiler/compiler.dart:282: help: 'Hoist types from class signatures',
defaultsTo: false, hide: true)
On 2016/08/12 20:30:57, nweiz wrote:
> Long line.

Done.

[Jennifer Messerly, 2016-08-12 21:09:23]

https://codereview.chromium.org/2244703003/diff/80001/lib/src/compiler/comman...
File lib/src/compiler/command.dart (right):

https://codereview.chromium.org/2244703003/diff/80001/lib/src/compiler/comman...
lib/src/compiler/command.dart:18: final ArgParser _argParser = () {
In case you're curious, our type inference would totally infer this, but someone
(Paul Berry?) added this warning:

WARNING: Unsafe use of block closure in a type-inferred variable outside a
function body.  Workaround: add a type annotation for `_argParser`.  See
dartbug.com/26947 ([dev_compiler] lib/src/compiler/command.dart:18)

[nweiz, 2016-08-12 21:16:38]

lgtm

https://codereview.chromium.org/2244703003/diff/40001/bin/dartdevc.dart
File bin/dartdevc.dart (right):

https://codereview.chromium.org/2244703003/diff/40001/bin/dartdevc.dart#newco...
bin/dartdevc.dart:58: // it at some point.
On 2016/08/12 21:08:01, John Messerly wrote:
> On 2016/08/12 20:30:57, nweiz wrote:
> > Is it really important to be backwards compatible here? I doubt you have any
> > external users who are relying strongly on existing behavior, and IIRC most
of
> > the time the command structure existed it didn't even work when invoked from
> the
> > SDK.
> 
> our internal build rules still pass it

In that case, consider filing bugs to use the new API and linking those bugs
here.

https://codereview.chromium.org/2244703003/diff/40001/lib/src/compiler/comman...
File lib/src/compiler/command.dart (right):

https://codereview.chromium.org/2244703003/diff/40001/lib/src/compiler/comman...
lib/src/compiler/command.dart:76: _print("```");
On 2016/08/12 21:08:01, John Messerly wrote:
> As a general rule I try and not fix-all-the-things when doing a refactor.

My philosophy is, if I don't fix incidental stuff as part of other changes, I'll
never find time to fix them at all. :)

https://codereview.chromium.org/2244703003/diff/80001/lib/src/compiler/comman...
File lib/src/compiler/command.dart (right):

https://codereview.chromium.org/2244703003/diff/80001/lib/src/compiler/comman...
lib/src/compiler/command.dart:34: /// The runs a single compile for dartdevc.
Remove "The".

[Jennifer Messerly, 2016-08-12 21:58:42]

thanks!

https://codereview.chromium.org/2244703003/diff/40001/bin/dartdevc.dart
File bin/dartdevc.dart (right):

https://codereview.chromium.org/2244703003/diff/40001/bin/dartdevc.dart#newco...
bin/dartdevc.dart:58: // it at some point.
On 2016/08/12 21:16:37, nweiz wrote:
> On 2016/08/12 21:08:01, John Messerly wrote:
> > On 2016/08/12 20:30:57, nweiz wrote:
> > > Is it really important to be backwards compatible here? I doubt you have
any
> > > external users who are relying strongly on existing behavior, and IIRC
most
> of
> > > the time the command structure existed it didn't even work when invoked
from
> > the
> > > SDK.
> > 
> > our internal build rules still pass it
> 
> In that case, consider filing bugs to use the new API and linking those bugs
> here.

CC'd you on my internal CL to fix it. I'm going to optimistically assume it
lands and remove this code :)

https://codereview.chromium.org/2244703003/diff/40001/bin/dartdevc.dart#newco...
bin/dartdevc.dart:79: return new Future.value(new WorkResponse()
On 2016/08/12 20:30:57, nweiz wrote:
> Nit, but if a function doesn't do any async work but has to return a future to
> fit an interface, I usually annotate it "async" rather than using "new
> Future.value". That way you can refactor the function internals all you want
> without messing with the semantics.

Done.

https://codereview.chromium.org/2244703003/diff/80001/lib/src/compiler/comman...
File lib/src/compiler/command.dart (right):

https://codereview.chromium.org/2244703003/diff/80001/lib/src/compiler/comman...
lib/src/compiler/command.dart:34: /// The runs a single compile for dartdevc.
On 2016/08/12 21:16:38, nweiz wrote:
> Remove "The".

Eeep, good catch!

[Jennifer Messerly, 2016-08-12 22:06:47]

Description was changed from

==========
fixes #610, incorrect help output
==========

to

==========
fixes #610, incorrect help output

R=nweiz@google.com

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

[Jennifer Messerly, 2016-08-12 22:06:48]

Committed patchset #7 (id:120001) manually as
a242daa56131a4dedf6293b8a20086e74eb468ed (presubmit successful).