Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

impl(generator/protobuf): optional and repeated fields #74

Conversation

coryan
Copy link
Contributor

@coryan coryan commented Nov 2, 2024

Support repeated and optional fields. Maybe the largest contribution here is to
introduce a way to unit test the Protobuf translator.

@coryan coryan marked this pull request as ready for review November 2, 2024 14:34
@coryan coryan force-pushed the impl-generator-protobuf-support-repeated-and-optional-fields branch from 128e8dc to e37b936 Compare November 3, 2024 13:07
Support repeated and optional fields. Maybe the largest contribution here is to
introduce a way to unit test the Protobuf translator.

api := translator.makeAPI()

if message := api.State.MessageByID[".test.Fake"]; message != nil {
Copy link
Member

Choose a reason for hiding this comment

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

I think that this will panic if message is nil. Instead do:

message, ok := api.StateMessageById[".test.Fake"]
if !ok {
    t.Fatalf("Cannot find message %s in API State", "Fake")
}

checkMessage(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Name: "Fake",
Documentation: "A test message.",
Fields: []*genclient.Field{
{Repeated: true, Documentation: "A repeated field tag = 1", Name: "f_double", JSONName: "fDouble", ID: ".test.Fake.f_double", Typez: genclient.DOUBLE_TYPE},
Copy link
Member

Choose a reason for hiding this comment

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

ditto re indent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@coryan coryan force-pushed the impl-generator-protobuf-support-repeated-and-optional-fields branch from e37b936 to 06902d4 Compare November 4, 2024 14:43

func TestScalar(t *testing.T) {
contents := `
syntax = "proto3";
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think this would be easier to read if we made this its own file in a testdata folder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, but it would be harder to know what files go with what test. I find it easier to change the tests if the inputs, test code, and expected outputs are all in one place.

I will defer to you an Julie on Golang stuff in any case.

})
}

func newCodeGeneratorRequest(name, contents string, t *testing.T) *pluginpb.CodeGeneratorRequest {
Copy link
Member

Choose a reason for hiding this comment

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

nit: generally I would expect to see t as the first parameter to the test. Also to improve test reporting should there be a failure consider marking this function as a helper function. You would call t.Helper at the start of the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@coryan coryan enabled auto-merge (squash) November 4, 2024 15:09
@coryan coryan merged commit e74d91d into googleapis:main Nov 4, 2024
9 checks passed
@coryan coryan deleted the impl-generator-protobuf-support-repeated-and-optional-fields branch November 4, 2024 15:11
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.

3 participants