Initial support for generating client and server stubs for Dart.

Initial support for generating client and server stubs for Dart.

R=sgjesse@google.com, skybrian@google.com
BUG=

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

[wibling, 2015-06-23 11:11:52]

wibling@google.com changed reviewers:
+ cbracken@google.com

[Søren Gjesse, 2015-06-23 12:08:31]

https://chromiumcodereview.appspot.com/1196293003/diff/1/lib/client_generator...
File lib/client_generator.dart (right):

https://chromiumcodereview.appspot.com/1196293003/diff/1/lib/client_generator...
lib/client_generator.dart:21: : '${parent.classname}_${descriptor.name}',
Can services be nested? If so please add a test for that case.

https://chromiumcodereview.appspot.com/1196293003/diff/1/lib/file_generator.dart
File lib/file_generator.dart (right):

https://chromiumcodereview.appspot.com/1196293003/diff/1/lib/file_generator.d...
lib/file_generator.dart:102: 'import \'dart:async\';\n'
Is there any way to only import this if needed?

We already have the problem with the two package imports below causing warnings
when not used.

[wibling, 2015-06-23 13:26:35]

Thanks for the review. Updated diff.

https://chromiumcodereview.appspot.com/1196293003/diff/1/lib/client_generator...
File lib/client_generator.dart (right):

https://chromiumcodereview.appspot.com/1196293003/diff/1/lib/client_generator...
lib/client_generator.dart:21: : '${parent.classname}_${descriptor.name}',
On 2015/06/23 12:08:31, Søren Gjesse wrote:
> Can services be nested? If so please add a test for that case.

No, I have simplified this to just descriptor.name.

https://chromiumcodereview.appspot.com/1196293003/diff/1/lib/file_generator.dart
File lib/file_generator.dart (right):

https://chromiumcodereview.appspot.com/1196293003/diff/1/lib/file_generator.d...
lib/file_generator.dart:102: 'import \'dart:async\';\n'
On 2015/06/23 12:08:31, Søren Gjesse wrote:
> Is there any way to only import this if needed?
> 
> We already have the problem with the two package imports below causing
warnings
> when not used.

I will take a look and see what I can come up with.

[Søren Gjesse, 2015-06-24 09:08:03]

lgtm

https://chromiumcodereview.appspot.com/1196293003/diff/60001/lib/client_gener...
File lib/client_generator.dart (right):

https://chromiumcodereview.appspot.com/1196293003/diff/60001/lib/client_gener...
lib/client_generator.dart:36: out.println('ProtobufClient _client;');
We could just make this 'var' then the class name can change... - or put in
RpcClient as we agreed upon.

https://chromiumcodereview.appspot.com/1196293003/diff/60001/pubspec.yaml
File pubspec.yaml (right):

https://chromiumcodereview.appspot.com/1196293003/diff/60001/pubspec.yaml#new...
pubspec.yaml:9: protobuf: '>=0.3.8 <0.4.0'
0.3.10 now.

[skybrian, 2015-06-24 16:16:30]

Apparently I missed actually sending these.

The API is not right. Let's roll back and try again.

https://chromiumcodereview.appspot.com/1196293003/diff/60001/lib/client_gener...
File lib/client_generator.dart (right):

https://chromiumcodereview.appspot.com/1196293003/diff/60001/lib/client_gener...
lib/client_generator.dart:45: '\'/${_descriptor.name}.${m.name}\',
request.writeToBuffer());');
It seems like ProtobufClient should have the responsibility for constructing
whatever URL it needs, so just pass in the descriptor name and method name as
separate arguments here?

Also, a PbClient might be using JSON instead of a binary transport, so I think
we should pass it two GeneratedMessage instances for the request and response,
and let PbClient do the serialization/deserialization?

(So the only work we're really doing here is calling the appropriate
GeneratedMessage subclass constructor for the response, which would be difficult
for PbClient to do.)

https://chromiumcodereview.appspot.com/1196293003/diff/60001/lib/indenting_wr...
File lib/indenting_writer.dart (right):

https://chromiumcodereview.appspot.com/1196293003/diff/60001/lib/indenting_wr...
lib/indenting_writer.dart:15: void addBlock(String start, String end, void
body(), {endWithNewline: true}) {
Is endWithNewline used?

https://chromiumcodereview.appspot.com/1196293003/diff/60001/lib/service_gene...
File lib/service_generator.dart (right):

https://chromiumcodereview.appspot.com/1196293003/diff/60001/lib/service_gene...
lib/service_generator.dart:24: ? '.${descriptor.name}'
This calculation is a bit messy. Perhaps move it to a _getFullName() function,
and then you can use early returns instead of nested ?:.

https://chromiumcodereview.appspot.com/1196293003/diff/60001/lib/service_gene...
lib/service_generator.dart:46: 'Future<List<int>> handleCall(ServerContext ctx,
String method, '
We should make this handle JSON and binary on the server side too, which seems
tricky.

One way would be to change List<int> to dynamic for both request and response,
and also pass in two methods:

typedef DeserializeFunc(request, GeneratedMessage msg);

typedef SerializeFunc(GeneratedMessage msg, response);

That seems awkward, so another way would be to use a GeneratedMessage for both
request and response, but then we need another method for the caller to use to
create the request:

GeneratedMessage createRequest(String method);

Future<GeneratedMessage> handleCall(ServerContext ctx, String method,
GeneratedMessage request);

https://chromiumcodereview.appspot.com/1196293003/diff/60001/lib/service_gene...
lib/service_generator.dart:49: out.addBlock('if (method == \'${m.name}\') {', '}
else ', () {
Not a big deal, but it seems like a switch statement would be nicer.

https://chromiumcodereview.appspot.com/1196293003/diff/60001/lib/service_gene...
lib/service_generator.dart:57: out.println('throw \'Unknown method:
\$method\';');
make this an ArgumentError?

[skybrian, 2015-06-24 23:57:23]

Hi, I made the changes for wire protocol independence myself.

I'm unable to upload the patch here since I'm not owner, so I uploaded them to a
different issue:

https://chromiumcodereview.appspot.com/1196293003/

[wibling, 2015-06-25 12:50:51]

PTAL, now with Brian's changes and nits addressed

[Søren Gjesse, 2015-06-25 13:04:49]

LGTM

Let's land this!

https://chromiumcodereview.appspot.com/1196293003/diff/160001/test/service_te...
File test/service_test.dart (right):

https://chromiumcodereview.appspot.com/1196293003/diff/160001/test/service_te...
test/service_test.dart:11: Future<pb.SearchResponse> search(ServerContext ctx,
pb.SearchRequest request) async {
Long line.

[wibling, 2015-06-25 13:10:05]

Landing.

https://chromiumcodereview.appspot.com/1196293003/diff/160001/test/service_te...
File test/service_test.dart (right):

https://chromiumcodereview.appspot.com/1196293003/diff/160001/test/service_te...
test/service_test.dart:11: Future<pb.SearchResponse> search(ServerContext ctx,
pb.SearchRequest request) async {
On 2015/06/25 13:04:49, Søren Gjesse wrote:
> Long line.

Done.

[wibling, 2015-06-25 13:10:44]

Committed patchset #9 (id:160001) manually as
f33e6e8a92f9220cd952c807d61dcacc128f311a (presubmit successful).