Switch polymer's build.dart to use the new linter. This CL does the following

Switch polymer's build.dart to use the new linter. This CL does the following
changes:
  - deletes the old compiler entirely and switches over to the new linter. 
  - adds a new build_helper.dart (to replace components_build.dart)
  - defines 'lint' and 'deploy' methods that can be called from build.dart
  - includes options to limit the scope of what's an entrypoint, and to override
    the currentPackage and packageDirs
  - includes the '--deploy' flag to address: dartbug.com/13050

R=jmesserly@google.com

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

[Siggi Cherem (dart-lang), 2013-09-10 03:40:57]

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/deploy.dart
File pkg/polymer/lib/deploy.dart (left):

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/deploy.dar...
pkg/polymer/lib/deploy.dart:98: Future _run(String outDir, bool includeTests) {
most code below here basically moved to lib/src/barback_helper.dart

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/polymer_el...
File pkg/polymer/lib/polymer_element.dart (right):

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/polymer_el...
pkg/polymer/lib/polymer_element.dart:19: import 'src/utils.dart' show
toCamelCase, toHyphenedName;
the old utils is gone, the new utils is what utils_observe used to be

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/src/barbac...
File pkg/polymer/lib/src/barback_helper.dart (right):

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/src/barbac...
pkg/polymer/lib/src/barback_helper.dart:22: /** Collects different parameters
needed to configure and run barback. */
this configuration and runBarback are new.

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/src/barbac...
pkg/polymer/lib/src/barback_helper.dart:68: String
readCurrentPackageFromPubspec() {
the logic here and below is the same as before, with minor tweaks to use the
'options' object where necessary.

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/src/messag...
File pkg/polymer/lib/src/messages.dart (left):

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/src/messag...
pkg/polymer/lib/src/messages.dart:5: library messages;
gone!

[Jennifer Messerly, 2013-09-10 04:16:51]

Awesome stuff! <3 seeing so much code cleanup!

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
File pkg/polymer/lib/build_helper.dart (right):

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:64: *     main() {
this looks like the most common pattern. let's combine?

main() => build(['web/index.html']);

where "build" is defined as: lint and deploy if requested. Options can be
supplied but if not, it will parse them.

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:71: library build_helper;
library polymer.build_helper;

actually -- instead of _helper could we just call it "build" or "builder" ?

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:77: import
'package:polymer/src/barback_helper.dart';
this should be 'src/barback_helper.dart' for consistency

(or we should use package:polymer/src everywhere...)

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:105: : new Linter(linterOptions);
slight improvement:

var formatter = options.machineFormat ? jsonFormatter : null;
var linter = new Linter(linterOptions, formatter);

(you could also add .formatter getter to options, make it read even better)

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:108: for (var asset in assets) {
how stable is this order? wondering if we should we sort these before printing

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:112: if (!content.isEmpty) print(content);
i'm not sure we want to print from an async callback. the order of messages will
be undefined. consider using Future.wait (or whatever it's called now)

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:120: * Runs the polymer deploy step.
"Creates a directory suitable for deploying a Polymer application to a server" 
?

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:122: * The [entrypoints] list contains files
under web/ that should be treated as
must they be under "web/"?

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:124: * (for example 'web/index.html'). If
emtpy, all files under 'web/' are treated
very minor quibble: i'm not sure about having 0 wraparound to max. perhaps
letting "null" be the signal for "select everything"

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:130: * Internally, the deploy script needs to
know the name of the [currentPackage]
trivial edit: I think you can remove "internally"
also "function" instead of "script"?

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:135: * Note: we expect this function to
disappear in the future when `pub deploy`
**Note**: this function will be replaced in the future by the `pub deploy`
command.

put at the beginning?

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:158: List<String> changedFiles;
final fields?

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:214: help: 'currently a noop, may be used in
the future to remove any build'
long line in help? consider adding \n

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:221: ..addOption('out', abbr: 'o', help:
'Directory where to generate files.',
nit: noticed this one was capitalized and has a period but the others do not. I
don't have a stylistic preference -- suggest seeing what other tools do -- but
it would be nice to be consistent.

My quick survey of Linux command line tools didn't show a case preference,
although they all avoid periods unless it had multiple sentences.

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:233: return new
CommandLineOptions(results['changed'], results['removed'],
trivial: this might be a good case for a shortish variable name

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/deploy.dart
File pkg/polymer/lib/deploy.dart (right):

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/deploy.dar...
pkg/polymer/lib/deploy.dart:20: * **Note**: If you already have a `build.dart`
in your application, we
suggestion: move up front

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/deploy.dar...
pkg/polymer/lib/deploy.dart:94: ..addOption('out', abbr: 'o', help: 'Directory
where to generate files.',
"directory to generate files" ?
or "directory to generate files into"

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/deploy.dar...
pkg/polymer/lib/deploy.dart:114: print('Usage: dart --package-root=packages/ '
is the --package-root actually required? even for packages/ ?

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/polymer_el...
File pkg/polymer/lib/polymer_element.dart (right):

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/polymer_el...
pkg/polymer/lib/polymer_element.dart:19: import 'src/utils.dart' show
toCamelCase, toHyphenedName;
On 2013/09/10 03:40:57, Siggi Cherem (dart-lang) wrote:
> the old utils is gone, the new utils is what utils_observe used to be

yeah, i noticed the eyesore "utils_observe" recently too. yay!

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/src/barbac...
File pkg/polymer/lib/src/barback_helper.dart (right):

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/src/barbac...
pkg/polymer/lib/src/barback_helper.dart:9: library polymer.src.barback_helper;
i'm not a huge fan of _helper, could we just call it barback?

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/src/barbac...
pkg/polymer/lib/src/barback_helper.dart:19: import 'package:args/args.dart';
sort if you want :)

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/src/barbac...
pkg/polymer/lib/src/barback_helper.dart:105: final Set<String>
_polymerPackageDependencies = (const [
fyi -- I don't think you need the parens.

also since this is lazy initialized anyway (the .toSet call) I don't think
"const" buys anything.

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/src/transf...
File pkg/polymer/lib/src/transform/common.dart (right):

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/src/transf...
pkg/polymer/lib/src/transform/common.dart:45: bool isHtmlEntrypoint(AssetId id)
{
nit: entry point is two words? so entryPoint

(funny example:
http://msdn.microsoft.com/en-us/library/system.entrypointnotfoundexception.aspx)

[Siggi Cherem (dart-lang), 2013-09-11 01:45:26]

Thanks John, PTAL

PatchSet 3 merges in your changes + renames files, but PatchSet 2 has all the
other changes so it's easier to see them side-by-side.

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
File pkg/polymer/lib/build_helper.dart (right):

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:64: *     main() {
On 2013/09/10 04:16:51, John Messerly wrote:
> this looks like the most common pattern. let's combine?
> 
> main() => build(['web/index.html']);
> 
> where "build" is defined as: lint and deploy if requested. Options can be
> supplied but if not, it will parse them.

Cool idea, added it for now. I think once pub deploy is here, then there is no
point in us having anything to do with '--deploy', then we would just have
'lint'.

I guess at that point build.dart should probably be renamed to something else
too.

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:71: library build_helper;
On 2013/09/10 04:16:51, John Messerly wrote:
> library polymer.build_helper;
> 
> actually -- instead of _helper could we just call it "build" or "builder" ?

Agree, Note I uploaded 2 patch sets. I did all the other changes first (Patch
Set 2) and then just renamed files (Patch Set 3). So hopefully that will help
show diffs more easily.

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:77: import
'package:polymer/src/barback_helper.dart';
On 2013/09/10 04:16:51, John Messerly wrote:
> this should be 'src/barback_helper.dart' for consistency
> 
> (or we should use package:polymer/src everywhere...)

oops, done.

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:105: : new Linter(linterOptions);
On 2013/09/10 04:16:51, John Messerly wrote:
> slight improvement:
> 
> var formatter = options.machineFormat ? jsonFormatter : null;
> var linter = new Linter(linterOptions, formatter);
> 
> (you could also add .formatter getter to options, make it read even better)

Done.

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:108: for (var asset in assets) {
On 2013/09/10 04:16:51, John Messerly wrote:
> how stable is this order? wondering if we should we sort these before printing

good point. added sorting below.

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:112: if (!content.isEmpty) print(content);
On 2013/09/10 04:16:51, John Messerly wrote:
> i'm not sure we want to print from an async callback. the order of messages
will
> be undefined. consider using Future.wait (or whatever it's called now)

Good point. did so below too.

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:120: * Runs the polymer deploy step.
On 2013/09/10 04:16:51, John Messerly wrote:
> "Creates a directory suitable for deploying a Polymer application to a server"

> ?

Done.

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:122: * The [entrypoints] list contains files
under web/ that should be treated as
On 2013/09/10 04:16:51, John Messerly wrote:
> must they be under "web/"?

Good question. For now that was the convention I was going after, but we can
change that.

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:124: * (for example 'web/index.html'). If
emtpy, all files under 'web/' are treated
On 2013/09/10 04:16:51, John Messerly wrote:
> very minor quibble: i'm not sure about having 0 wraparound to max. perhaps
> letting "null" be the signal for "select everything"

Sorry, that is how it's implemented (null = max), fixed the doc.

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:130: * Internally, the deploy script needs to
know the name of the [currentPackage]
On 2013/09/10 04:16:51, John Messerly wrote:
> trivial edit: I think you can remove "internally"
> also "function" instead of "script"?

Done, but used "step" instead of "function".

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:135: * Note: we expect this function to
disappear in the future when `pub deploy`
On 2013/09/10 04:16:51, John Messerly wrote:
> **Note**: this function will be replaced in the future by the `pub deploy`
> command.
> 
> put at the beginning?

sgtm

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:158: List<String> changedFiles;
On 2013/09/10 04:16:51, John Messerly wrote:
> final fields?

Done.

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:214: help: 'currently a noop, may be used in
the future to remove any build'
On 2013/09/10 04:16:51, John Messerly wrote:
> long line in help? consider adding \n

reworded it.

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:221: ..addOption('out', abbr: 'o', help:
'Directory where to generate files.',
On 2013/09/10 04:16:51, John Messerly wrote:
> nit: noticed this one was capitalized and has a period but the others do not.
I
> don't have a stylistic preference -- suggest seeing what other tools do -- but
> it would be nice to be consistent.
> 
> My quick survey of Linux command line tools didn't show a case preference,
> although they all avoid periods unless it had multiple sentences.

good catch. I normally like lowercase + no period, but seems like 'dart2js' and
'pub' do the opposite. So I changed all the help messages.

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/build_help...
pkg/polymer/lib/build_helper.dart:233: return new
CommandLineOptions(results['changed'], results['removed'],
On 2013/09/10 04:16:51, John Messerly wrote:
> trivial: this might be a good case for a shortish variable name

Done.

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/deploy.dart
File pkg/polymer/lib/deploy.dart (right):

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/deploy.dar...
pkg/polymer/lib/deploy.dart:20: * **Note**: If you already have a `build.dart`
in your application, we
On 2013/09/10 04:16:51, John Messerly wrote:
> suggestion: move up front

Done.

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/deploy.dar...
pkg/polymer/lib/deploy.dart:94: ..addOption('out', abbr: 'o', help: 'Directory
where to generate files.',
On 2013/09/10 04:16:51, John Messerly wrote:
> "directory to generate files" ?
> or "directory to generate files into"

Done.

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/deploy.dar...
pkg/polymer/lib/deploy.dart:114: print('Usage: dart --package-root=packages/ '
On 2013/09/10 04:16:51, John Messerly wrote:
> is the --package-root actually required? even for packages/ ?

Yeah, this is because the 'package-root' is resolved from the entrypoint,
without any regard of the current working dir. Maybe worth filing a bug about
it?

The current error looks like this:
Unable to open file: package:polymer/packages/polymer/deploy.dart

Which seems as if they are treating "package:polymer/deploy.dart" as a path, and
resolving package-root to: "package:polymer/packages"

pretty wacky.

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/src/barbac...
File pkg/polymer/lib/src/barback_helper.dart (right):

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/src/barbac...
pkg/polymer/lib/src/barback_helper.dart:9: library polymer.src.barback_helper;
On 2013/09/10 04:16:51, John Messerly wrote:
> i'm not a huge fan of _helper, could we just call it barback?

Not sure if that's too confusing. Other ideas:
- utils_barback
- barback_utils
- barback_runner

Currently leaning towards the third one. I did this rename in PatchSet 3
(PatchSet 2 kept this file in place and did all the other fixes).

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/src/barbac...
pkg/polymer/lib/src/barback_helper.dart:19: import 'package:args/args.dart';
On 2013/09/10 04:16:51, John Messerly wrote:
> sort if you want :)

doh. we need an autoformatter...

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/src/barbac...
pkg/polymer/lib/src/barback_helper.dart:105: final Set<String>
_polymerPackageDependencies = (const [
On 2013/09/10 04:16:51, John Messerly wrote:
> fyi -- I don't think you need the parens.
> 
> also since this is lazy initialized anyway (the .toSet call) I don't think
> "const" buys anything.

good point. Done.

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/src/transf...
File pkg/polymer/lib/src/transform/common.dart (right):

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/src/transf...
pkg/polymer/lib/src/transform/common.dart:45: bool isHtmlEntrypoint(AssetId id)
{
On 2013/09/10 04:16:51, John Messerly wrote:
> nit: entry point is two words? so entryPoint
> 
> (funny example:
>
http://msdn.microsoft.com/en-us/library/system.entrypointnotfoundexception.aspx)

Done.

[Jennifer Messerly, 2013-09-11 20:07:53]

lgtm

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/deploy.dart
File pkg/polymer/lib/deploy.dart (right):

https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/deploy.dar...
pkg/polymer/lib/deploy.dart:114: print('Usage: dart --package-root=packages/ '
On 2013/09/11 01:45:26, Siggi Cherem (dart-lang) wrote:
> On 2013/09/10 04:16:51, John Messerly wrote:
> > is the --package-root actually required? even for packages/ ?
> 
> Yeah, this is because the 'package-root' is resolved from the entrypoint,
> without any regard of the current working dir. Maybe worth filing a bug about
> it?
> 
> The current error looks like this:
> Unable to open file: package:polymer/packages/polymer/deploy.dart
> 
> Which seems as if they are treating "package:polymer/deploy.dart" as a path,
and
> resolving package-root to: "package:polymer/packages"
> 
> pretty wacky.
> 

strange :| perhaps we should just tell people to run 

packages/polymer/deploy.dart? or does that have the same problem?

[Siggi Cherem (dart-lang), 2013-09-11 20:23:44]

On 2013/09/11 20:07:53, John Messerly wrote:
> lgtm
> 
> https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/deploy.dart
> File pkg/polymer/lib/deploy.dart (right):
> 
>
https://codereview.chromium.org/23898009/diff/6001/pkg/polymer/lib/deploy.dar...
> pkg/polymer/lib/deploy.dart:114: print('Usage: dart --package-root=packages/ '
> On 2013/09/11 01:45:26, Siggi Cherem (dart-lang) wrote:
> > On 2013/09/10 04:16:51, John Messerly wrote:
> > > is the --package-root actually required? even for packages/ ?
> > 
> > Yeah, this is because the 'package-root' is resolved from the entrypoint,
> > without any regard of the current working dir. Maybe worth filing a bug
about
> > it?
> > 
> > The current error looks like this:
> > Unable to open file: package:polymer/packages/polymer/deploy.dart
> > 
> > Which seems as if they are treating "package:polymer/deploy.dart" as a path,
> and
> > resolving package-root to: "package:polymer/packages"
> > 
> > pretty wacky.
> > 
> 
> strange :| perhaps we should just tell people to run 
> 
> packages/polymer/deploy.dart? or does that have the same problem?

same problem =(

[Siggi Cherem (dart-lang), 2013-09-11 20:38:49]

Committed patchset #3 manually as r27403 (presubmit successful).