Skip to content

Add optional annotations to some Result fields for memory actions.#204

Closed
CRefice wants to merge 1 commit intogoogle:masterfrom
CRefice:crefice/memory-action-proto-optional
Closed

Add optional annotations to some Result fields for memory actions.#204
CRefice wants to merge 1 commit intogoogle:masterfrom
CRefice:crefice/memory-action-proto-optional

Conversation

@CRefice
Copy link
Collaborator

@CRefice CRefice commented Mar 18, 2026

This makes field presence behave as expected.

This makes field presence behave as expected.
@panhania
Copy link
Member

Could you expand by what you mean by "behave as expected"? In general, I am not fan of optional (this is why I stick to proto3) and try to design all my protos so that it is not required. So far, we don't have a single optional field in the whole RRG codebase, so introducing them here feels weird.

@CRefice
Copy link
Collaborator Author

CRefice commented Mar 18, 2026

Right, should have expanded. The problem I have is that many of those fields are, quoting the comments, Set only if error is not set. I think therefore it makes sense to mark those as optional both semantically, and so that field presence is set correctly for those fields.

@panhania
Copy link
Member

I think therefore it makes sense to mark those as optional both semantically (...)

We are using proto3, they are optional semantically.

so that field presence is set correctly for those fields

Do we have a specific use case for using additional field presence of these specific fields? For me using it is a backward compatibility footgun. Otherwise you could argue that you should always just include optional but clearly this is not a good idea since it was removed from proto3 for a reason (and then brought back to accommodate for specific use cases).

@CRefice
Copy link
Collaborator Author

CRefice commented Mar 18, 2026

Fair enough. I had some use cases in mind, but as I've been writing and rewriting the server-side flow I realized they were unnecessary. I'll close this PR for now.

@CRefice CRefice closed this Mar 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants