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

Use lower_camel_case consistently for fields in protobuf definitions #257

Merged
merged 5 commits into from
Jun 29, 2023

Conversation

joshuagl
Copy link
Contributor

We claim to adhere to the protobuf programming practices, and thus should also follow the style guide there.

The style guide states that message field names should be lower_snake_case, not lowerCamelCase. Our protobuf definitions are somewhat inconsistent, this PR normalises to lower_snake_case for field names in protobuf definitions.

Fixes: #250

Convert from lowerCamelCase to lower_snake_case. This is the protobuf
convention and recommended by the style guide:
https://protobuf.dev/programming-guides/style/#message-field-names

Signed-off-by: Joshua Lock <joshua.lock@uk.verizon.com>
Convert from lowerCamelCase to lower_snake_case. This is the protobuf
convention and recommended by the style guide:
https://protobuf.dev/programming-guides/style/#message-field-names

Signed-off-by: Joshua Lock <joshua.lock@uk.verizon.com>
Convert from lowerCamelCase to lower_snake_case. This is the protobuf
convention and recommended by the style guide:
https://protobuf.dev/programming-guides/style/#message-field-names

Signed-off-by: Joshua Lock <joshua.lock@uk.verizon.com>
Document that the specification text and the protobuf definitions use
different field case formats (lowerCamelCase vs. lower_snake_case).

Signed-off-by: Joshua Lock <joshua.lock@uk.verizon.com>
The docs claimed that "it's typical to keep generated Go code in the
repository itself", only --- we generate multiple types of code (Go,
Java, and Python at present). Make the statement more ambiguous to
convey that we will store multiple types of generated code in the repo
and that contributors should not push any generated code themselves.

Signed-off-by: Joshua Lock <joshua.lock@uk.verizon.com>
@joshuagl joshuagl requested a review from a team as a code owner June 27, 2023 12:11
Copy link
Contributor

@marcelamelara marcelamelara left a comment

Choose a reason for hiding this comment

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

Thanks @joshuagl ! These changes shouldn't break the tests, right?

@joshuagl
Copy link
Contributor Author

Thanks @joshuagl ! These changes shouldn't break the tests, right?

This PR does not appear to break the tests.
Alas, the tests are already broken. I've proposed a fix in #259. With that fix and this branch combined, the tests pass.

Copy link
Contributor

@TomHennen TomHennen left a comment

Choose a reason for hiding this comment

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

So the generated API stays the same? Seems like a win.

@joshuagl
Copy link
Contributor Author

Alas, no. The generated API does change. Presumably the changed API is not covered by current tests, though? make go_test succeeds after my change is merged (it even uses cached results, further indicating the changed API is not covered by the tests).

@marcelamelara
Copy link
Contributor

Hmm, the changed APIs should be covered in the tests AFAIK (I wrote them), in that the tests do some (un)marshalling of RD and Statement protos. I wonder now if the JSON protobuf API used in the tests treats the updated API as equivalent? I can look into this, but since the tests are working fine, we can probably leave this for another PR.

@joshuagl
Copy link
Contributor Author

Apologies, it turns out I was wrong. I just did a manual review of the proto changes with this series applied, you can see the diff I've reviewed in a gist https://gist.github.com/joshuagl/ae743664f4422391d6e88010b1d167c0

The Go API is unchanged: struct names remain the same

@adityasaky
Copy link
Member

This is interesting:

-	DownloadLocation string                     `protobuf:"bytes,5,opt,name=downloadLocation,proto3" json:"downloadLocation,omitempty"`
-	MediaType        string                     `protobuf:"bytes,6,opt,name=mediaType,proto3" json:"mediaType,omitempty"`
+	DownloadLocation string                     `protobuf:"bytes,5,opt,name=download_location,json=downloadLocation,proto3" json:"download_location,omitempty"`
+	MediaType        string                     `protobuf:"bytes,6,opt,name=media_type,json=mediaType,proto3" json:"media_type,omitempty"`

If I'm parsing this right, when the proto is snake cased, the json tag supports JSON docs with snake cased keys but also the camelcased ones (I know there's default conversion somewhere in there).

Thinking about it more, I guess we knew this because in-toto v1 links are snake cased but there are proto definitions that support them...

@joshuagl joshuagl merged commit 7aefca3 into in-toto:main Jun 29, 2023
1 check passed
@joshuagl joshuagl deleted the joshuagl/proto-field-case branch June 29, 2023 16:09
marcelamelara added a commit to marcelamelara/in-toto.attestation that referenced this pull request Jul 10, 2023
* Make parameters explicit in constructors; use in tests
* Add type annotations in functions
* Update constructors based on in-toto#257 and in-toto#263

Signed-off-by: Marcela Melara <marcela.melara@intel.com>
marcelamelara added a commit to marcelamelara/in-toto.attestation that referenced this pull request Jul 12, 2023
* Make parameters explicit in constructors; use in tests
* Add type annotations in functions
* Update constructors based on in-toto#257 and in-toto#263

Signed-off-by: Marcela Melara <marcela.melara@intel.com>
marcelamelara added a commit to marcelamelara/in-toto.attestation that referenced this pull request Aug 1, 2023
* Make parameters explicit in constructors; use in tests
* Add type annotations in functions
* Update constructors based on in-toto#257 and in-toto#263

Signed-off-by: Marcela Melara <marcela.melara@intel.com>
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.

CamelCased protos
4 participants