-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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
doc: add guidance for RPC to developer notes #30142
base: master
Are you sure you want to change the base?
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
doc/developer-notes.md
Outdated
|
||
A few guidelines for modifying and reviewing existing RPC interfaces: | ||
|
||
- When modifying RPC interface JSON structure, implement associated `-deprecatedrpc=` option to retain previous RPC behavior, including (but not limited to) the following: data types changes (e.g. from `{"result":{"warnings":""}}` to `{"result":{"warnings":[]}}`, changing a value from a string to a number, etc.), logical meaning changes of a value, key name changes (e.g. `{"result":{"warning":""}}` to `{"result":{"warnings":""}}`). Include detailed instructions on feature deprecation and re-enabling in release notes. |
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.
nit: this sentence might be better structured starting with a verb:
Implement associated `-deprecatedrpc=` option to retain previous RPC behavior when modifying RPC interface JSON structure, including (but not limited to) the following:
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. Agreed. Updated.
doc/developer-notes.md
Outdated
@@ -1449,6 +1449,16 @@ A few guidelines for introducing and reviewing new RPC interfaces: | |||
- *Rationale*: JSON strings are Unicode strings, not byte strings, and | |||
RFC8259 requires JSON to be encoded as UTF-8. | |||
|
|||
- When choosing data types for RPC JSON, prefer data types that are flexible to expansion without change of data type. For example, `{"result":{"warnings":[]}}` rather than `{"result":{"warnings":""}}`, which allows for multiple warnings to be included as array elements. |
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.
nit: this sentence might be better structured starting with a verb:
Prefer to choose RPC JSON data types that are flexible to expansion without change of data type.
ACK 61853a2. |
doc/developer-notes.md
Outdated
@@ -1449,6 +1449,16 @@ A few guidelines for introducing and reviewing new RPC interfaces: | |||
- *Rationale*: JSON strings are Unicode strings, not byte strings, and | |||
RFC8259 requires JSON to be encoded as UTF-8. | |||
|
|||
- When choosing data types for RPC JSON, prefer data types that are flexible to expansion without change of data type. For example, `{"result":{"warnings":[]}}` rather than `{"result":{"warnings":""}}`, which allows for multiple warnings to be included as array elements. |
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.
how is this different from "- Try to make the RPC response a JSON object."?
The "warnings"
example here doesn't make much sense, because both are equally flexible to expansion. (See the plural s
.) The reason why array should be preferred over string is that array is the correct type to represent an array. (But that is obvious, isn't it?)
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 a look.
Good point. Removed this statement as it is quite similar to Try to make the RPC response a JSON object
and could be confusing.
Adds deprecatedrpc guidance statement to the RPC interface guidelines section.
Adds guidance statements to the RPC interface section of the developer notes with examples of when to implement
-deprecatedrpc=
.Wanted to increase awareness of preferred RPC implementation approaches for newer contributors.
This implements some of what's discussed in #29912 (comment)
Opinions may differ, so please don't be shy. We want to make RPC as solid/safe as possible.
Example of a discussion where guidelines/context might have added value:
#29845 (comment)