-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Proto Auditing: Replace ID with Id #7032
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7032 +/- ##
==========================================
+ Coverage 61.56% 61.98% +0.41%
==========================================
Files 421 520 +99
Lines 25463 32015 +6552
==========================================
+ Hits 15677 19843 +4166
- Misses 8433 10542 +2109
- Partials 1353 1630 +277 |
this is sad from an aesthetic point of view 😢 , but understandable. Could we open an issue to have a short ADR on why we don't use "ID" so future implementers can be aware of the potential issue. Literally could be a copy paste of what you wrote in the pr description, but then I could link the ADR in the IBC code spec's ref on why ADR is useful here |
@colin-axner yes, see #7033. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you update IBC specs 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
closes #7033
Description
In proto files, don't allow
customname
which renamesId
toID
.Rationale
In our proto files, we sometimes use gogoproto's
customname
tag to rename fields, e.g.ClientId
toClientID
, to fit what the Go linter recommends. In the generated*.pb.go
files, we indeed see fields with...ID
(capitalized).In #6918, we are adding grpc-gateway, who generates other files
*.pb.gw.go
. However, grpc-gateway ignores gogoproto'scustomname
tag, and outputs fields with...Id
(PascalCase). See here for an example.The Go compiler then obviously complains.
Potential solutions
./scripts
to manually changeId
->ID
inside grpc-gateway-generated files*.pb.gw.go
.custonname
on Id renaming.This PR implements 3.
Notes
...ID
in variable names and elsewhere in the code, so this PR creates some small inconsistencies in namings.custonnames
(e.g.Ed25519->ED25519
,Url->URL
are kept). When generating grpc-gateway files, they didn't seem to interfere. This PR only changesID->Id
, as these were the ones that made the compiler shout.Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes