Disambiguate names of service methods in grpc output #487
Conversation
protoc_plugin/lib/names.dart
Outdated
| ..._serviceNames, | ||
| ]; | ||
|
|
||
| final List<String> _serviceNames = <String>[ |
There was a problem hiding this comment.
Can this be const? Also, do we need the explicit type on the left-hand side?
There was a problem hiding this comment.
yes!
Done - also in surrounding code.
| @@ -30,6 +30,18 @@ class TestClient extends $grpc.Client { | |||
| '/Test/Bidirectional', | |||
| ($0.Input value) => value.writeToBuffer(), | |||
| ($core.List<$core.int> value) => $0.Output.fromBuffer(value)); | |||
| static final _$new_ = $grpc.ClientMethod<$0.Input, $0.Output>( | |||
There was a problem hiding this comment.
One thing I was curious of, is it possible to have a name collision with the library import aliases? i.e. do we also need to add r'$grpc', r'$async' etc to the reserved words?
There was a problem hiding this comment.
$ cannot occur in a protobuf identifier, (https://developers.google.com/protocol-buffers/docs/reference/proto3-spec#identifiers) - I also tried it out and the protoc compiler gave a syntax error.
I believe that is why we chose to use $ in the first place for these names.
That means we could actually remove all the reserved words with $!
I won't do that in this PR to keep the scope smaller.
There was a problem hiding this comment.
(I removed it from those added).
|
I realized we also needed disambiguation for the .pbserver.dart code, and added that - this will require a bit of integration work when moving into google3. @nichite @iinozemtsev do you know if we have any real uses of the pbserver.dart files internally? Searching for 'import.*.pbserver.dart' gives no real results. Maybe it is time to retire that code, and have the grpc code be generated by default? |
|
I agree with retiring the .pbserver files, I don't see any internally. I was thinking the same thing about the generated RPC client stubs that protobuf puts out that aren't functional. For those, there are one or two subclassed internally, but I was thinking they both could just be replaced by gRPC. |
|
Possible I was doing branch cleanup and didn't realize this was NOT the null safety migration branch. |
|
Reconstructed in #625 |
Fixes: #485