-
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
R4R: Message & Codec Registration Consistency #3673
Conversation
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.
I really like these changes. This PR is, however, breaking. We just need to think about which release to land this in.
Codecov Report
@@ Coverage Diff @@
## develop #3673 +/- ##
===========================================
+ Coverage 61.15% 61.18% +0.02%
===========================================
Files 190 190
Lines 14008 14008
===========================================
+ Hits 8567 8571 +4
+ Misses 4905 4901 -4
Partials 536 536 |
@jackzampolin indeed we do. I suppose for 0.33? |
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.
Existing changes look good, but:
ag validator_src_addr
pulls up a few more instances in REST bech-JSON structs- I think we should use
_
in tags to be consistent with JSON (cc @rigelrozanski though who might disagree)
@cwgoes thanks for the catch! |
Also maybe this PR should include a style guide so we have this convention written down somewhere. |
This is now failing tests and has merge conflicts :( |
@jackzampolin @rigelrozanski @alessio @cwgoes updated |
Needs a rebase, unfortunately. |
ACK pending rebase. |
Updated @cwgoes |
closes: #3669
/cc @kwunyeung (please let me know if I missed anything)
Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (
docs/
)Added entries in
PENDING.md
with issue #rereviewed
Files changed
in the github PR explorerFor Admin Use: