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

Replace clang-format with buf format in proto-format #3651

Open
2 of 3 tasks
DimitrisJim opened this issue May 25, 2023 · 1 comment
Open
2 of 3 tasks

Replace clang-format with buf format in proto-format #3651

DimitrisJim opened this issue May 25, 2023 · 1 comment
Labels
needs discussion Issues that need discussion before they can be worked on protobuf Proto file definitions and protobuf code generation

Comments

@DimitrisJim
Copy link
Contributor

Summary

For formatting proto files, we currently use clang-format. Afaik, this was inherited from the sdk with #2672. Alternatively, we could consider using buf which is already used in the Makefile for the proto-lint and proto-check-breaking rules.

See #3640 (comment) for some context on why this issue was opened.

Pros

  • It doesn't require an additional tool (though clang-format is a pretty popular binary) be present.
  • It formats import paths
  • We already use it for other makefile rules.

Cons

  • Might make merging new sdk prs slightly more involved.
  • Doesn't offer customizability options and the formatting buf currently does is opinionated, to say the least.

Here's a sample diff from formatting client.proto with buf:

 package ibc.core.client.v1;
 
-option go_package = "github.com/cosmos/ibc-go/v7/modules/core/02-client/types";
-
-import "gogoproto/gogo.proto";
-import "google/protobuf/any.proto";
 import "cosmos/upgrade/v1beta1/upgrade.proto";
 import "cosmos_proto/cosmos.proto";
+import "gogoproto/gogo.proto";
+import "google/protobuf/any.proto";
+
+option go_package = "github.com/cosmos/ibc-go/v7/modules/core/02-client/types";
 
 // IdentifiedClientState defines a client state with an additional client
 // identifier field.
@@ -41,7 +41,7 @@ message ClientConsensusStates {
 // handler may fail if the subject and the substitute do not match in client and
 // chain parameters (with exception to latest height, frozen height, and chain-id).
 message ClientUpdateProposal {
-  option (gogoproto.goproto_getters)         = false;
+  option (gogoproto.goproto_getters) = false;
   option (cosmos_proto.implements_interface) = "cosmos.gov.v1beta1.Content";
   // the title of the update proposal
   string title = 1;
@@ -57,14 +57,14 @@ message ClientUpdateProposal {
 // UpgradeProposal is a gov Content type for initiating an IBC breaking
 // upgrade.
 message UpgradeProposal {
-  option (gogoproto.goproto_getters)         = false;
-  option (gogoproto.goproto_stringer)        = false;
-  option (gogoproto.equal)                   = true;
+  option (gogoproto.goproto_getters) = false;
+  option (gogoproto.goproto_stringer) = false;
+  option (gogoproto.equal) = true;
   option (cosmos_proto.implements_interface) = "cosmos.gov.v1beta1.Content";
 
-  string                      title       = 1;
-  string                      description = 2;
-  cosmos.upgrade.v1beta1.Plan plan        = 3 [(gogoproto.nullable) = false];
+  string title = 1;
+  string description = 2;
+  cosmos.upgrade.v1beta1.Plan plan = 3 [(gogoproto.nullable) = false];
 
   // An UpgradedClientState must be provided to perform an IBC breaking upgrade.
   // This will make the chain commit to the correct upgraded (self) client state
@@ -86,7 +86,7 @@ message UpgradeProposal {
 // height continues to be monitonically increasing even as the RevisionHeight
 // gets reset
 message Height {
-  option (gogoproto.goproto_getters)  = false;
+  option (gogoproto.goproto_getters) = false;
   option (gogoproto.goproto_stringer) = false;
 
   // the revision that the client is currently on

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@DimitrisJim DimitrisJim added the protobuf Proto file definitions and protobuf code generation label May 25, 2023
@damiannolan
Copy link
Member

Hmm, I must admit I do kind of like the formatting clang-format provides wrt to spacing and aligning field number ordering.
But I'd be open to looking into this a bit more 👍

@damiannolan damiannolan added the needs discussion Issues that need discussion before they can be worked on label May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion Issues that need discussion before they can be worked on protobuf Proto file definitions and protobuf code generation
Projects
None yet
Development

No branches or pull requests

2 participants