feat: use proto presence information to inform nullability#43
feat: use proto presence information to inform nullability#43devoncarew wants to merge 1 commit intogoogleapis:mainfrom
Conversation
| print('<No textual response>'); | ||
| } else { | ||
| print(parts.map((p) => p.text ?? '').join('')); | ||
| print('<= ' + parts.map((p) => p.text).join('')); |
There was a problem hiding this comment.
The nice thing is that you won't need the map here because p.text can never be null. But text is part of a oneof. How can you tell if that field is set or not? The Python API makes it nullable: https://github.com/googleapis/python-genai/blob/7e0beba3afb4329660d7377c5276194bf68cbcef/google/genai/types.py#L1293
There was a problem hiding this comment.
I don't think we really support oneofs for the Dart backend yet? Or at least not explicitly - perhaps we mostly do as long as everything is nullable.
I can do some checks for oneof and keep those fields nullable. We may also want to write out whether a field is part of a oneof (w/ at least one other field)? So for text:
/// Inline text.
///
/// Only one of [text] and [inlineData] may be set.
final String? text;
| print('=> $prompt'); | ||
| final result = await service.generateContent(request); | ||
| final parts = result.candidates?[0].content?.parts; | ||
| final parts = result.candidates[0].content?.parts; |
There was a problem hiding this comment.
[optional]
| final parts = result.candidates[0].content?.parts; | |
| final parts = result.candidates.first.content?.parts; |
|
|
||
| final result = await service.generateContent(request); | ||
| final parts = result.candidates?[0].content?.parts; | ||
| final parts = result.candidates[0].content?.parts; |
There was a problem hiding this comment.
[optional]
| final parts = result.candidates[0].content?.parts; | |
| final parts = result.candidates.first.content?.parts; |
|
|
||
| await for (final result in service.streamGenerateContent(request)) { | ||
| final parts = result.candidates?[0].content?.parts; | ||
| final parts = result.candidates[0].content?.parts; |
There was a problem hiding this comment.
[optional]
| final parts = result.candidates[0].content?.parts; | |
| final parts = result.candidates.first.content?.parts; |
| ); | ||
| } | ||
|
|
||
| @override | ||
| Object toJson() { | ||
| return {'cachedContent': cachedContent.toJson()}; | ||
| return { | ||
| if (cachedContent != null) 'cachedContent': cachedContent!.toJson(), |
There was a problem hiding this comment.
We can avoid the !
| if (cachedContent != null) 'cachedContent': cachedContent!.toJson(), | |
| if (cachedContent case final content?) 'cachedContent': content.toJson(), |
There was a problem hiding this comment.
Let's track that in #41; the PR for the generator changes already has lots of things going on, and I haven't fully groked pattens myself.
| updateMask: decodeCustom(json['updateMask'], FieldMask.fromJson), | ||
| ); | ||
| } | ||
|
|
||
| @override | ||
| Object toJson() { | ||
| return { | ||
| 'cachedContent': cachedContent.toJson(), | ||
| if (cachedContent != null) 'cachedContent': cachedContent!.toJson(), |
There was a problem hiding this comment.
| if (cachedContent != null) 'cachedContent': cachedContent!.toJson(), | |
| if (cachedContent case final content?) 'cachedContent': content.toJson(), |
| } | ||
|
|
||
| extension BoolExtension on bool { | ||
| bool get isNotDefault => this != false; |
There was a problem hiding this comment.
This looks like a good choice to me. There is a downside in having this API that leaks into auto-complete all over, but I think I still prefer it over the result.hasFoo ? result.roo : null; type API.
There was a problem hiding this comment.
Thanks! The good news here is that the generated code uses these extensions in their implementations, but the extensions aren't currently in anything that the user would import. So the symbols won't be visible to the users by default.
3 participants
Use proto presence information to inform nullability:
This is a draft PR as I'm still working on the generator portion of the changes. I think this is still interesting to look at, esp. at how it effects the code using the APIs (the examples, tests, ...).
I'll include more info in a non-draft PR, or in the generator PR, but:
optional, that promotes it to explicit presence; you can determine whether a value had been set or not. We generate those as nullable Dart fields.We're making the decision in this generator to not serialize to json fields that have implicit presence and where they have the default value for that type. I believe this is correct, but it would be good to get independent confirmation / some trial and error here to get a bit more confidence.
cc @brianquinlan and @natebosch