changes for 0.3.9

changes for 0.3.9

- Modify dart_options support so that it supports alternate mixins.
- Move the experimental map implementation to PbMapMixin

For now, new mixins can only be added using a patch:
  - add the new class to the protobuf library
  - add the class to the list in mixin.dart.

R=sgjesse@google.com, sigmund@google.com

Committed: https://github.com/dart-lang/dart-protoc-plugin/commit/8195898175228cb9fcd10856ededc39e3664d77b

[skybrian, 2015-06-20 00:56:55]

Requires corresponding change to protobuf library
https://chromiumcodereview.appspot.com/1196773004

The idea is that we will experiment with alternative mixins in google3, without
needing to open source the changes. The PbMapMixin class serves as an example to
ensure that the hook works.

Providing full support for mixins (without patching the protobuf package) is
trickier since we need a way to add the appropriate imports to the .pb.dart file
and the appropriate dependencies to the pub package.

[skybrian, 2015-06-22 17:17:51]

skybrian@google.com changed reviewers:
+ sgjesse@chromium.org, sigmund@google.com

[Søren Gjesse, 2015-06-22 18:29:37]

sgjesse@google.com changed reviewers:
+ sgjesse@google.com

[Søren Gjesse, 2015-06-22 18:29:38]

lgtm

https://chromiumcodereview.appspot.com/1192943003/diff/1/lib/code_generator.dart
File lib/code_generator.dart (right):

https://chromiumcodereview.appspot.com/1192943003/diff/1/lib/code_generator.d...
lib/code_generator.dart:37: var request = new
CodeGeneratorRequest.fromBuffer(bytes, extensions);
Long line.

https://chromiumcodereview.appspot.com/1192943003/diff/1/lib/message_generato...
File lib/message_generator.dart (right):

https://chromiumcodereview.appspot.com/1192943003/diff/1/lib/message_generato...
lib/message_generator.dart:118: mixinClause = " with ${mixin}";
The new approach currently does not require/support additional imports?

https://chromiumcodereview.appspot.com/1192943003/diff/1/lib/src/dart_options...
File lib/src/dart_options.pb.dart (right):

https://chromiumcodereview.appspot.com/1192943003/diff/1/lib/src/dart_options...
lib/src/dart_options.pb.dart:8: //import 'descriptor_2_5_opensource.pb.dart' as
google$protobuf;
Code in comment.

[Siggi Cherem (dart-lang), 2015-06-22 19:38:57]

lgtm too, some minor suggestions belo

https://chromiumcodereview.appspot.com/1192943003/diff/1/lib/mixin.dart
File lib/mixin.dart (right):

https://chromiumcodereview.appspot.com/1192943003/diff/1/lib/mixin.dart#newcode5
lib/mixin.dart:5: part of protoc;
might be worth renaming this file, maybe make it it's own library under
lib/src/? 

For example, `lib/src/reserved_names.dart`.

https://chromiumcodereview.appspot.com/1192943003/diff/1/lib/mixin.dart#newcode8
lib/mixin.dart:8: // TODO: figure out how to extract the reserved names
automatically.
since protoc runs in the vm only, you could to do automatically with mirrors.

https://chromiumcodereview.appspot.com/1192943003/diff/1/lib/mixin.dart#newco...
lib/mixin.dart:10: "PbMapMixin": PbMapMixin_reservedNames,
nit: unindent (indent only +2)

[skybrian, 2015-06-23 02:21:26]

https://chromiumcodereview.appspot.com/1192943003/diff/1/lib/code_generator.dart
File lib/code_generator.dart (right):

https://chromiumcodereview.appspot.com/1192943003/diff/1/lib/code_generator.d...
lib/code_generator.dart:37: var request = new
CodeGeneratorRequest.fromBuffer(bytes, extensions);
On 2015/06/22 18:29:38, Søren Gjesse wrote:
> Long line.

Done.

https://chromiumcodereview.appspot.com/1192943003/diff/1/lib/message_generato...
File lib/message_generator.dart (right):

https://chromiumcodereview.appspot.com/1192943003/diff/1/lib/message_generato...
lib/message_generator.dart:118: mixinClause = " with ${mixin}";
On 2015/06/22 18:29:38, Søren Gjesse wrote:
> The new approach currently does not require/support additional imports?

It didn't because the PbMapMixin was in the protobuf package which is already
imported. However, since I moved it, I had to implement import handling again.

https://chromiumcodereview.appspot.com/1192943003/diff/1/lib/mixin.dart
File lib/mixin.dart (right):

https://chromiumcodereview.appspot.com/1192943003/diff/1/lib/mixin.dart#newcode5
lib/mixin.dart:5: part of protoc;
On 2015/06/22 19:38:57, Siggi Cherem (dart-lang) wrote:
> might be worth renaming this file, maybe make it it's own library under
> lib/src/? 
> 
> For example, `lib/src/reserved_names.dart`.

I replaced this file in favor of the mixins_meta library in the protobuf
package.

https://chromiumcodereview.appspot.com/1192943003/diff/1/lib/mixin.dart#newcode8
lib/mixin.dart:8: // TODO: figure out how to extract the reserved names
automatically.
On 2015/06/22 19:38:57, Siggi Cherem (dart-lang) wrote:
> since protoc runs in the vm only, you could to do automatically with mirrors.

Yes, but I'm not sure why we didn't do that before for the reserved names in
GeneratedMessage, so I went with existing practice for now.

https://chromiumcodereview.appspot.com/1192943003/diff/1/lib/src/dart_options...
File lib/src/dart_options.pb.dart (right):

https://chromiumcodereview.appspot.com/1192943003/diff/1/lib/src/dart_options...
lib/src/dart_options.pb.dart:8: //import 'descriptor_2_5_opensource.pb.dart' as
google$protobuf;
On 2015/06/22 18:29:38, Søren Gjesse wrote:
> Code in comment.

Removed.
(protoc generates a file containing this import, but it's not actually used
anywhere, so I removed it.)

[Søren Gjesse, 2015-06-23 06:43:33]

lgtm

https://chromiumcodereview.appspot.com/1192943003/diff/20001/lib/file_generat...
File lib/file_generator.dart (right):

https://chromiumcodereview.appspot.com/1192943003/diff/20001/lib/file_generat...
lib/file_generator.dart:108: out.println("import '${imp}' show ${symbols.join(',
')};");
This import could cause name-clashes. Should these imports always be generated
using a unique 'as' name?

[skybrian, 2015-06-23 17:32:42]

https://chromiumcodereview.appspot.com/1192943003/diff/20001/lib/file_generat...
File lib/file_generator.dart (right):

https://chromiumcodereview.appspot.com/1192943003/diff/20001/lib/file_generat...
lib/file_generator.dart:108: out.println("import '${imp}' show ${symbols.join(',
')};");
On 2015/06/23 06:43:33, Søren Gjesse wrote:
> This import could cause name-clashes. Should these imports always be generated
> using a unique 'as' name?

It's a good point but I'm going to defer this, since we don't allow arbitrary
mixins to be used yet.

Name conflicts should be very rare because the name of a mixin class usually
ends with "Mixin" and proto messages are unlikely to have names ending with
"Mixin". (I did a code search and found one example.)

[skybrian, 2015-06-23 17:35:35]

Committed patchset #2 (id:20001) manually as
8195898175228cb9fcd10856ededc39e3664d77b (presubmit successful).

[Søren Gjesse, 2015-06-24 07:11:27]

https://chromiumcodereview.appspot.com/1192943003/diff/20001/lib/file_generat...
File lib/file_generator.dart (right):

https://chromiumcodereview.appspot.com/1192943003/diff/20001/lib/file_generat...
lib/file_generator.dart:108: out.println("import '${imp}' show ${symbols.join(',
')};");
On 2015/06/23 17:32:42, skybrian wrote:
> On 2015/06/23 06:43:33, Søren Gjesse wrote:
> > This import could cause name-clashes. Should these imports always be
generated
> > using a unique 'as' name?
> 
> It's a good point but I'm going to defer this, since we don't allow arbitrary
> mixins to be used yet.
> 
> Name conflicts should be very rare because the name of a mixin class usually
> ends with "Mixin" and proto messages are unlikely to have names ending with
> "Mixin". (I did a code search and found one example.)

Please file an issue on this.

[skybrian, 2015-06-24 18:46:53]

On 2015/06/24 07:11:27, Søren Gjesse wrote:
>
https://chromiumcodereview.appspot.com/1192943003/diff/20001/lib/file_generat...
> File lib/file_generator.dart (right):
> 
>
https://chromiumcodereview.appspot.com/1192943003/diff/20001/lib/file_generat...
> lib/file_generator.dart:108: out.println("import '${imp}' show
${symbols.join(',
> ')};");
> On 2015/06/23 17:32:42, skybrian wrote:
> > On 2015/06/23 06:43:33, Søren Gjesse wrote:
> > > This import could cause name-clashes. Should these imports always be
> generated
> > > using a unique 'as' name?
> > 
> > It's a good point but I'm going to defer this, since we don't allow
arbitrary
> > mixins to be used yet.
> > 
> > Name conflicts should be very rare because the name of a mixin class usually
> > ends with "Mixin" and proto messages are unlikely to have names ending with
> > "Mixin". (I did a code search and found one example.)
> 
> Please file an issue on this.

Done, see: https://github.com/dart-lang/dart-protoc-plugin/issues/45