feat: use proto presence information to inform nullability#58
feat: use proto presence information to inform nullability#58devoncarew wants to merge 6 commits 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.
Is this example right? The generated Part has a nullable text field:
final class Part extends ProtoMessage {
static const String fullyQualifiedName =
'google.ai.generativelanguage.v1beta.Part';
/// Inline text.
final String? text;Which is what I'd expect because it is part of a OneOf.
There was a problem hiding this comment.
No - good catch. text is nullable, but we're assuming has content based on the context of the sample. I added a 'bang' (!).
print(parts.map((p) => p.text!).join(''));- add `isNotDefault` extension methods for core proto types This is the hand-written portions of #58. I'll follow this up w/ generator changes (in the librarian repo), and then a PR back to this repo that will re-generate the packages and update test and example code (the rest of #58). Happy to bike-shed on the extension method name(s). It's `isNotDefault` now, but could be something like `notDefault`, ... . cc @natebosch
brianquinlan
left a comment
There was a problem hiding this comment.
This is going to be a pain to merge.
For package versions, I think that the easiest thing to do is:
- remove all of the
version =from all .sidekick.toml except the one at the root - update the root .sidekick.toml "# Generated packages section" such that all of the version constraints are '^0.2.0'
Not only is that expedient, but I think it is also correct ;-)
Let me know if you want me to do the merge work.
|
|
||
| final result = await service.generateContent(request); | ||
| final parts = result.candidates?[0].content?.parts; | ||
| final parts = result.candidates.first.content?.parts; |
There was a problem hiding this comment.
| final parts = result.candidates.first.content?.parts; | |
| final parts = result.candidates.firstOrNull?.content?.parts; |
| final parts = result.candidates.first.content?.parts; | ||
| if (parts != null) { | ||
| parts.forEach((p) => stdout.write(p.text ?? '')); | ||
| parts.forEach((p) => stdout.write(p.text)); |
There was a problem hiding this comment.
| parts.forEach((p) => stdout.write(p.text)); | |
| parts.forEach((p) => stdout.write(p.text!)); |
I think? Could you rerun these examples.
|
|
||
| final result = await service.generateContent(request); | ||
| final parts = result.candidates?[0].content?.parts; | ||
| final parts = result.candidates.first.content?.parts; |
There was a problem hiding this comment.
| final parts = result.candidates.first.content?.parts; | |
| final parts = result.candidates.firstOrNull?.content?.parts; |
|
|
||
| final result = await service.generateContent(request); | ||
| final parts = result.candidates?[0].content?.parts; | ||
| final parts = result.candidates.first.content?.parts; |
There was a problem hiding this comment.
| final parts = result.candidates.first.content?.parts; | |
| final parts = result.candidates.firstOrNull?.content?.parts; |
What I was thinking of - once the upstream sidekick change lands - was to create a new PR w/ the latest generated code. I.e., not trying to update this PR. I'll then make the same changes to the examples and tests that this PR has. Those were all straightforward - just addressing issues reported by the type system. |
Use proto presence info to generate null-safe code: - `annotateField` updated to use information from proto optional annotations - toJson() implementations write values if they're nullable and have a non-null value, or if they're not nullable and have a non-default value for that type - fromJson parses a value for parse a value for nullable fields if one is given; it will parse any value for non-null field or fall back to using the default value for that type - properly dereference null values when referring to request fields for use in url paths - enums definitions now get a default value (`EnumClass.$default`) - removed the older annotate.requiredFields and tests (based on usage in service calls) - closes googleapis/google-cloud-dart#24 Tests are updated for `buildQueryLines()` to include optional fields. Tests were added for the `createFromJsonLine()` and `createToJsonLine()` methods; previously we got our testing here from being on the same repo as the dart code, and running the dart unit tests on the generator's putput. In-lined from the PR: > - proto 3 fields default to implicit presence > - the 'optional' keyword changes a field to explicit presence > - types like lists (repeated) and maps are always implicit presence > > Explicit presence means that you can know whether the user set a value or > not. Implicit presence means you can always retrieve a value; if one had > not been set, you'll see the default value for that type. > > We translate explicit presence (a optional annotation) to using a nullable > type for that field. We translate implicit presence (always returning some > value) to a non-null type. > > Some short-hand: > - optional == explicit == nullable > - implicit == non-nullable > - lists and maps == implicit == non-nullable > - singular message == explicit == nullable Sample output can be seen at googleapis/google-cloud-dart#58.
Update the packages here using the null-safety generator change: - a follow up to googleapis/librarian#2726 - update sidekick version reference - re-generate the code - update the hand-written code (part files, examples, and tests) Previously as a draft at #58; done as separate commits to make review a little easier. @brianquinlan, this does not include sidekick / dart package version changes. I assume we'll want to bump everything (to 0.2.0?). Not sure if we're bumping on changes or on publish (or don't have a specific process yet).
An update to #43; this is still a draft PR.