no longer require entry points to be specified in the build.dart file

no longer require entry points to be specified in the build.dart file

BUG= http://dartbug.com/20396
R=sigmund@google.com

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

[jakemac, 2014-09-15 20:34:14]

jakemac@google.com changed reviewers:
+ sigmund@google.com

[Siggi Cherem (dart-lang), 2014-09-15 21:35:20]

https://codereview.chromium.org/569393002/diff/20001/pkg/polymer/lib/builder....
File pkg/polymer/lib/builder.dart (right):

https://codereview.chromium.org/569393002/diff/20001/pkg/polymer/lib/builder....
pkg/polymer/lib/builder.dart:331: if (!pubspec.existsSync()) {
one option here - instead of using the CWD, use the script path:

path.join(
  path.dirname(Platform.script.path),
  'pubspec.yaml')

https://codereview.chromium.org/569393002/diff/20001/pkg/polymer/lib/builder....
pkg/polymer/lib/builder.dart:338: if (transformers is! List ||
transformers.any((t) => t is! Map)) {
entries in the transformer can actually be strings too. For example,

transformers:
- polymer

is valid.

https://codereview.chromium.org/569393002/diff/20001/pkg/polymer/lib/builder....
pkg/polymer/lib/builder.dart:351: else if (parsedEntryPoints is! List) error =
true;
style nit: use { } in this and the previous line.

(we only use `if (x) something;` without '{ }' if it fits on a single line and
there is no else.)

https://codereview.chromium.org/569393002/diff/20001/pkg/polymer/lib/builder....
pkg/polymer/lib/builder.dart:352: if (parsedEntryPoints.any((e) => e is!
String)) error = true;
actually - to avoid code duplication, you could just make the 'readEntrypoints'
function in transformer.dart public, and use it here

https://codereview.chromium.org/569393002/diff/20001/pkg/polymer/lib/builder....
pkg/polymer/lib/builder.dart:359: entryPoints.addAll(parsedEntryPoints);
also return here? (once we find the polymer section, we should be done).

https://codereview.chromium.org/569393002/diff/20001/pkg/polymer/lib/builder_...
File pkg/polymer/lib/builder_init.dart (right):

https://codereview.chromium.org/569393002/diff/20001/pkg/polymer/lib/builder_...
pkg/polymer/lib/builder_init.dart:8: library polymer.builder.init;
maybe rename the file and library to 'default_build'?

https://codereview.chromium.org/569393002/diff/20001/pkg/polymer/lib/builder_...
pkg/polymer/lib/builder_init.dart:12: main(args) { build(options:
parseOptions(args)); }
I wonder if we should just call `lint`. `build` was more useful back in the day
when we were just getting started with pub-build, and people wanted an
alternative way to build. I don't think it's used much today, and it is not that
well tested.

[jakemac, 2014-09-15 22:20:53]

https://codereview.chromium.org/569393002/diff/20001/pkg/polymer/lib/builder....
File pkg/polymer/lib/builder.dart (right):

https://codereview.chromium.org/569393002/diff/20001/pkg/polymer/lib/builder....
pkg/polymer/lib/builder.dart:331: if (!pubspec.existsSync()) {
On 2014/09/15 21:35:20, Siggi Cherem (dart-lang) wrote:
> one option here - instead of using the CWD, use the script path:
> 
> path.join(
>   path.dirname(Platform.script.path),
>   'pubspec.yaml')

Done.

https://codereview.chromium.org/569393002/diff/20001/pkg/polymer/lib/builder....
pkg/polymer/lib/builder.dart:338: if (transformers is! List ||
transformers.any((t) => t is! Map)) {
On 2014/09/15 21:35:20, Siggi Cherem (dart-lang) wrote:
> entries in the transformer can actually be strings too. For example,
> 
> transformers:
> - polymer
> 
> is valid.

Done.

https://codereview.chromium.org/569393002/diff/20001/pkg/polymer/lib/builder....
pkg/polymer/lib/builder.dart:351: else if (parsedEntryPoints is! List) error =
true;
On 2014/09/15 21:35:20, Siggi Cherem (dart-lang) wrote:
> style nit: use { } in this and the previous line.
> 
> (we only use `if (x) something;` without '{ }' if it fits on a single line and
> there is no else.)

Done.

https://codereview.chromium.org/569393002/diff/20001/pkg/polymer/lib/builder....
pkg/polymer/lib/builder.dart:352: if (parsedEntryPoints.any((e) => e is!
String)) error = true;
On 2014/09/15 21:35:20, Siggi Cherem (dart-lang) wrote:
> actually - to avoid code duplication, you could just make the
'readEntrypoints'
> function in transformer.dart public, and use it here

Done.

https://codereview.chromium.org/569393002/diff/20001/pkg/polymer/lib/builder....
pkg/polymer/lib/builder.dart:359: entryPoints.addAll(parsedEntryPoints);
Technically, it works to have multiple different polymer transformer sections. I
can't think of the specific use case off the top of my head but there is no
reason to prevent it.

On 2014/09/15 21:35:20, Siggi Cherem (dart-lang) wrote:
> also return here? (once we find the polymer section, we should be done).

https://codereview.chromium.org/569393002/diff/20001/pkg/polymer/lib/builder_...
File pkg/polymer/lib/builder_init.dart (right):

https://codereview.chromium.org/569393002/diff/20001/pkg/polymer/lib/builder_...
pkg/polymer/lib/builder_init.dart:8: library polymer.builder.init;
On 2014/09/15 21:35:20, Siggi Cherem (dart-lang) wrote:
> maybe rename the file and library to 'default_build'?

Done.

https://codereview.chromium.org/569393002/diff/20001/pkg/polymer/lib/builder_...
pkg/polymer/lib/builder_init.dart:12: main(args) { build(options:
parseOptions(args)); }
On 2014/09/15 21:35:20, Siggi Cherem (dart-lang) wrote:
> I wonder if we should just call `lint`. `build` was more useful back in the
day
> when we were just getting started with pub-build, and people wanted an
> alternative way to build. I don't think it's used much today, and it is not
that
> well tested.

Done.

[Siggi Cherem (dart-lang), 2014-09-15 22:34:24]

one thing I just realized, otherwise lgtm

https://codereview.chromium.org/569393002/diff/60001/pkg/polymer/lib/builder....
File pkg/polymer/lib/builder.dart (right):

https://codereview.chromium.org/569393002/diff/60001/pkg/polymer/lib/builder....
pkg/polymer/lib/builder.dart:152: if (entryPoints == null) entryPoints =
_parseEntryPointsFromPubspec();
I just realized something - this is changing the default behavior we had from
before, which was that if no-entrypoints are listed, then everything under web/
is treated as an entrypoint. The fix might be to simply return 'null' from the
_parse function, instead of [], if we didn't find a specification for it.

[jakemac, 2014-09-15 22:49:48]

https://codereview.chromium.org/569393002/diff/60001/pkg/polymer/lib/builder....
File pkg/polymer/lib/builder.dart (right):

https://codereview.chromium.org/569393002/diff/60001/pkg/polymer/lib/builder....
pkg/polymer/lib/builder.dart:152: if (entryPoints == null) entryPoints =
_parseEntryPointsFromPubspec();
On 2014/09/15 22:34:24, Siggi Cherem (dart-lang) wrote:
> I just realized something - this is changing the default behavior we had from
> before, which was that if no-entrypoints are listed, then everything under
web/
> is treated as an entrypoint. The fix might be to simply return 'null' from the
> _parse function, instead of [], if we didn't find a specification for it. 

Done.

[jakemac, 2014-09-15 22:59:16]

Committed patchset #5 (id:80001) manually as 40286 (presubmit successful).