-
Couldn't load subscription status.
- Fork 17
COR-1759 Serde deprecated #318
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
base: main
Are you sure you want to change the base?
Conversation
7df48cb to
e8c4d78
Compare
e8c4d78 to
323233a
Compare
323233a to
490efba
Compare
|
|
||
| [features] | ||
| default = ["serde_deprecated"] | ||
| serde_deprecated = [] |
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.
Let's document also here in Cargo.toml that the flag means
| Updating these files should only be done when the node's API, determined by the | ||
| schemas, changes and we need to support the new API in the SDK. | ||
|
|
||
| The use of serde is guarded by the flag `serde_deprecated`. Enable the flag to use the sdk with serde. |
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.
We can be a bit more specific about the flag. Instead of "use of serde", it is serde::Serialize/Deserialize implementation that are guarded by the flag. And only on types where we consider it deprecated and it might be removed in the future.
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 don't think we need to feature flag the serialize/deserialize functions here, since they do not really promise a "default" serialization format of a specific type. They only have effect when used explicitly. Hence, I think we can just not feature flag them.
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.
There are formatting changes and some removed comments that should be reverted in this file
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.
was the previous formatter cargo +nightly-2023-04-01 fmt perhaps?
Purpose
Guard serde relevant constructs behind a feature flag eventually to be turned off by default
Changes
cd serde-refactor && cargo run -- ../src --dry-runto see candidates insrcsrc/**andexamples/**Checklist
hard-to-understand areas.
CLA acceptance
By submitting the contribution I accept the terms and conditions of the
Contributor License Agreement v1.0
link: https://developers.concordium.com/CLAs/Contributor-License-Agreement-v1.0.pdf
I accept the above linked CLA.