Connect up augmentations from macro output through to host. #21
Conversation
davidmorgan
force-pushed
the
macro-builder
branch
from
a5287c5 to
81605eb
Compare
davidmorgan
force-pushed
the
augment
branch
2 times, most recently
from
2a1e96e to
5fd74ba
Compare
| final augmentRequest = | ||
| AugmentRequest.fromJson(json.decode(request) as Map<String, Object?>); | ||
| // TODO(davidmorgan): support multiple macros. | ||
| final response = await macros.single.augment(host, augmentRequest); |
There was a problem hiding this comment.
Related to my comment in the previous PR but I don't think this is the correct API for us to expose, it is too low level. I would like to see macros look more like they do in the current API.
Also, I think this makes merging augmentations from multiple macros together more difficult, which is partly why the augmentation library generation is done by the host right now and not the macro.
pkgs/_macro_host/lib/macro_host.dart
Outdated
| @@ -61,18 +66,35 @@ class MacroHost implements MacroService { | |||
| return _macroPhases!; | |||
| } | |||
|
|
|||
| /// Sends it [request] to the macro with [name]. | |||
There was a problem hiding this comment.
| /// Sends it [request] to the macro with [name]. | |
| /// Sends [request] to the macro with [name]. |
| }); | ||
|
|
||
| /// Augmentation code. | ||
| String get code => node['code'] as String; |
There was a problem hiding this comment.
It seems wrong that this assumes code exists, but the constructor does not require it to be non-null. Should this be typed as String?? Similar question for below.
|
|
||
| @override | ||
| SyncJsonProvider get provide => (String path) { | ||
| if (path != 'file:///dart_model.schema.json') { |
There was a problem hiding this comment.
should we have a custom scheme for this?
| @@ -12,7 +12,10 @@ void main() { | |||
| for (final package in ['dart_model', 'macro_service']) { | |||
| test('$package output is up to date', () { | |||
| final expected = dart_model_generator.generate( | |||
| File('../../schemas/$package.schema.json').readAsStringSync()); | |||
| File('../../schemas/$package.schema.json').readAsStringSync(), | |||
| importDartModel: package == 'macro_service', | |||
There was a problem hiding this comment.
It would be good if we could somehow make this a more general thing, similar to how it works in protos where the dependencies live in the schema file. Is there an eventual path towards that?
davidmorgan
force-pushed
the
augment
branch
4 times, most recently
from
08f457e to
7d757a2
Compare
|
Thanks, PTAL. |
pkgs/_macro_host/lib/macro_host.dart
Outdated
| throw StateError('request is already pending'); | ||
| } | ||
| _responseCompleter = Completer<AugmentResponse>(); | ||
| final result = await _responseCompleter!.future; |
There was a problem hiding this comment.
Does this local variable here actually do anything? Maybe you are getting type promotion actually since its private so you don't need this (since the completer has a more specific type?).
What I actually had in mind was assigning the completer to a local variable (as well as the field) and returning the future from the local variable since it would have a better type. But if the promotion works here that is better.
Hook up macro augmentation output end to end.
Add the capability to reference the
dart_modelschema from themacro_serviceschema, becauseAugmentationis indart_model.The wire format still does not carry the actual request type because it's not needed yet, there are few enough request types to always "guess" correctly. It will need the request type very soon :)