-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
ADR 023 Protobuf Naming and Versioning #6083
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6083 +/- ##
==========================================
- Coverage 55.72% 54.83% -0.90%
==========================================
Files 450 444 -6
Lines 27036 26771 -265
==========================================
- Hits 15067 14681 -386
- Misses 10886 11048 +162
+ Partials 1083 1042 -41 |
Co-Authored-By: Marko <marbar3778@yahoo.com>
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.
As someone relatively new to the SDK codebase, I'm finding most of these changes to be an improvement on readability.
I get that we can avoid this being #api-breaking by making use of type aliases, but it would be nice to have an explicit strategy for whether we plan to maintain these type aliases indefinitely, or if they would get deprecated at some later point (6 months / 1 year in the future?).
Also, it would be nice to clarify whether new modules should create the corresponding type aliases for legacy naming conventions, or if that is considered necessary. In my opinion, i'd rather only include aliases where absolutely necessary (for backwards compatiblity of existing modules).
I don't have a strong preference here. As I've said in the ADR, I think |
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.
Thanks for taking the time to write this up @aaronc. In principle, I agree with the bulk of the proposal. On the few things I don't understand or disagree with, I've commented on 👍
I would like to get this merged. Anything I can help with @aaronc? Can you resolve any issues? |
Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
…to aaronc/proto-naming
@alexanderbez just updated this. Can you take a look and see if it looks good to move forward? |
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.
ACK
Description
In light of #6030 and our transition to
Any
, we can make our protobuf names just a bit more concise without going overboard and losing descriptiveness. We can also make our file organization a bit more standard so that clients have an easier time running codegen.For contributor use:
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerFor admin use:
WIP
,R4R
,docs
, etc)