Skip to content

Conversation

@brianquinlan
Copy link

No description provided.

@brianquinlan brianquinlan requested review from a team as code owners December 18, 2025 18:11
Copy link
Contributor

@coryan coryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of questions, but neither is blocking. I would like @vchudnov-g to take a look if possible.

Comment on lines +239 to +247
// The proto compiler seems to misgenerate optional bytes fields.
// The generated code looks like:
// ```
// if cmd.Flags().Changed("info.p_bytes") {
// RepeatDataBodyInfoInput.Info.PBytes = &repeatDataBodyInfoInputInfoPBytes
// }
// ```
// The dereference of `&repeatDataBodyInfoInputInfoPBytes` is incorrect since
// `repeatDataBodyInfoInputInfoPBytes` is already a `byte[]`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes! Did you file a bug?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not. I assumed that there was something weird about the tooling that Showcase is using since it seems impossible that the official golang proto compiler would misgenerate code for such a common type.

Comment on lines +1344 to +1346
if req.GetReturnPartialSuccess() {
params.Add("returnPartialSuccess", fmt.Sprintf("%v", req.GetReturnPartialSuccess()))
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know where these are coming from?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. I didn't look at any of the generated code.

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