Make servePackages's contents argument saner.

Make servePackages's contents argument saner.

R=rnystrom@google.com

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

[Bob Nystrom, 2014-08-20 23:50:23]

https://codereview.chromium.org/488323002/diff/1/pkg/scheduled_test/lib/sched...
File pkg/scheduled_test/lib/scheduled_process.dart (right):

https://codereview.chromium.org/488323002/diff/1/pkg/scheduled_test/lib/sched...
pkg/scheduled_test/lib/scheduled_process.dart:207: if (description == "pub get")
return;
You should check this in and immediately wipe it from your memory. Months from
now, this behavior will confuse the hell out of us!

https://codereview.chromium.org/488323002/diff/1/sdk/lib/_internal/pub/test/t...
File sdk/lib/_internal/pub/test/test_pub.dart (right):

https://codereview.chromium.org/488323002/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/test_pub.dart:258: /// (e.g. "foo-1.2.3") to their
contents.
Having the mention the package names and versions twice is verbose and
error-prone. A map of lists of pairs of maps to iterables is kind of hairy too.

I think it's time we made a little utility class here. What do you think of
something like:

servePackages([
  servedPackage("foo", "1.2.3", contents: [
    descriptor...
  ]),
  servedPackage("bar", "1.2.4")
]);

ServedPackage servedPackage(String name, String version, {Iterable<d.Descriptor>
contents}) { ... }

class ServedPackage {
  final String name;
  final Version version;
  final Iterable<d.Descriptor> contents
}

?

[nweiz, 2014-08-21 01:20:42]

Code review changes

[nweiz, 2014-08-21 01:21:23]

https://codereview.chromium.org/488323002/diff/1/sdk/lib/_internal/pub/test/t...
File sdk/lib/_internal/pub/test/test_pub.dart (right):

https://codereview.chromium.org/488323002/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/test_pub.dart:258: /// (e.g. "foo-1.2.3") to their
contents.
On 2014/08/20 23:50:23, Bob Nystrom wrote:
> Having the mention the package names and versions twice is verbose and
> error-prone. A map of lists of pairs of maps to iterables is kind of hairy
too.
> 
> I think it's time we made a little utility class here. What do you think of
> something like:
> 
> servePackages([
>   servedPackage("foo", "1.2.3", contents: [
>     descriptor...
>   ]),
>   servedPackage("bar", "1.2.4")
> ]);
> 
> ServedPackage servedPackage(String name, String version,
{Iterable<d.Descriptor>
> contents}) { ... }
> 
> class ServedPackage {
>   final String name;
>   final Version version;
>   final Iterable<d.Descriptor> contents
> }
> 
> ?

Done.

[Bob Nystrom, 2014-08-21 16:44:17]

I really like the builder pattern you've got here. Some suggestions, but LGTM!

https://codereview.chromium.org/488323002/diff/20001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/cache/add/package_not_found_test.dart (right):

https://codereview.chromium.org/488323002/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/cache/add/package_not_found_test.dart:13:
servePackages((_) {});
Make the builder optional?

https://codereview.chromium.org/488323002/diff/20001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/deps_test.dart (right):

https://codereview.chromium.org/488323002/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/deps_test.dart:15: builder.serve("normal", "1.2.3",
pubspec: {
In some of the other test functions, we have a "deps" named param for this.
Allowing a full pubspec is nice for some cases, but for the common case of just
specifying regular deps, that would be terser.

https://codereview.chromium.org/488323002/diff/20001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/serve_packages.dart (right):

https://codereview.chromium.org/488323002/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/serve_packages.dart:48: /// (e.g. "foo-1.2.3") to
their contents.
Remove this.

https://codereview.chromium.org/488323002/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/serve_packages.dart:51: /// will be served as well.
Remove this.

https://codereview.chromium.org/488323002/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/serve_packages.dart:53: {bool replace: false}) {
Ah, now I see why builder isn't optional. You *could* make two functions:
servePackages() and serveOnlyPackages() if you wanted.

https://codereview.chromium.org/488323002/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/serve_packages.dart:101: class PackageServerBuilder {
Doc comment here and below.

https://codereview.chromium.org/488323002/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/serve_packages.dart:103: var _futures = new
FutureGroup();
I know it's initialized here, but I think we still tend to annotate non-final
fields.

https://codereview.chromium.org/488323002/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/serve_packages.dart:151: return
_futures.future.then((_) => _futures = new FutureGroup());
The intent here isn't to return the FutureGroup, right? You're just using => for
brevity?

In most cases, that's fine, but I think here it might be clearer to use a {}
body just to make it obvious that FutureGroup isn't itself a Future that's being
deliberately awaited on again.

[nweiz, 2014-08-21 18:28:34]

Code review changes

[nweiz, 2014-08-21 18:28:52]

https://codereview.chromium.org/488323002/diff/20001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/cache/add/package_not_found_test.dart (right):

https://codereview.chromium.org/488323002/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/cache/add/package_not_found_test.dart:13:
servePackages((_) {});
On 2014/08/21 16:44:17, Bob Nystrom wrote:
> Make the builder optional?

It would have to be named, which makes the normal case a lot worse.

https://codereview.chromium.org/488323002/diff/20001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/deps_test.dart (right):

https://codereview.chromium.org/488323002/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/deps_test.dart:15: builder.serve("normal", "1.2.3",
pubspec: {
On 2014/08/21 16:44:17, Bob Nystrom wrote:
> In some of the other test functions, we have a "deps" named param for this.
> Allowing a full pubspec is nice for some cases, but for the common case of
just
> specifying regular deps, that would be terser.

Done.

https://codereview.chromium.org/488323002/diff/20001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/serve_packages.dart (right):

https://codereview.chromium.org/488323002/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/serve_packages.dart:48: /// (e.g. "foo-1.2.3") to
their contents.
On 2014/08/21 16:44:17, Bob Nystrom wrote:
> Remove this.

Done.

https://codereview.chromium.org/488323002/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/serve_packages.dart:51: /// will be served as well.
On 2014/08/21 16:44:17, Bob Nystrom wrote:
> Remove this.

Done.

https://codereview.chromium.org/488323002/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/serve_packages.dart:53: {bool replace: false}) {
On 2014/08/21 16:44:17, Bob Nystrom wrote:
> Ah, now I see why builder isn't optional. You *could* make two functions:
> servePackages() and serveOnlyPackages() if you wanted.

Done.

https://codereview.chromium.org/488323002/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/serve_packages.dart:101: class PackageServerBuilder {
On 2014/08/21 16:44:17, Bob Nystrom wrote:
> Doc comment here and below.

Done.

https://codereview.chromium.org/488323002/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/serve_packages.dart:103: var _futures = new
FutureGroup();
On 2014/08/21 16:44:17, Bob Nystrom wrote:
> I know it's initialized here, but I think we still tend to annotate non-final
> fields.

I don't think we're consistent either way. Why would we annotate when the type
is already right there, though?

https://codereview.chromium.org/488323002/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/serve_packages.dart:151: return
_futures.future.then((_) => _futures = new FutureGroup());
On 2014/08/21 16:44:17, Bob Nystrom wrote:
> The intent here isn't to return the FutureGroup, right? You're just using =>
for
> brevity?
> 
> In most cases, that's fine, but I think here it might be clearer to use a {}
> body just to make it obvious that FutureGroup isn't itself a Future that's
being
> deliberately awaited on again.

Done.

[nweiz, 2014-08-21 18:47:45]

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