Skip to content

Conversation

@AHaliq
Copy link

@AHaliq AHaliq commented Aug 28, 2025

Purpose

Guard serde relevant constructs behind a feature flag eventually to be turned off by default

Changes

  • created a tool to parse and transform rust AST to add the guards idempotently
  • run cd serde-refactor && cargo run -- ../src --dry-run to see candidates in src
  • apply transformation to src/** and examples/**

Checklist

  • My code follows the style of this project.
  • The code compiles without warnings.
  • I have performed a self-review of the changes.
  • I have documented my code, in particular the intent of the
    hard-to-understand areas.
  • (If necessary) I have updated the CHANGELOG.

CLA acceptance

By submitting the contribution I accept the terms and conditions of the
Contributor License Agreement v1.0

@AHaliq AHaliq requested a review from allanbrondum August 28, 2025 11:11
@AHaliq AHaliq self-assigned this Aug 28, 2025
@AHaliq AHaliq added [Size] Large [Type] Maintenance Cleanup or handling of technical debt. question Further information is requested Breaking change labels Aug 28, 2025
@AHaliq AHaliq marked this pull request as draft August 28, 2025 11:11
@AHaliq AHaliq force-pushed the serde_deprecated branch 6 times, most recently from 7df48cb to e8c4d78 Compare August 29, 2025 18:10
@AHaliq AHaliq marked this pull request as ready for review October 6, 2025 08:59
@AHaliq AHaliq changed the title [DRAFT] COR-1759 Serde deprecated COR-1759 Serde deprecated Oct 6, 2025

[features]
default = ["serde_deprecated"]
serde_deprecated = []
Copy link
Contributor

@allanbrondum allanbrondum Oct 6, 2025

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.
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Breaking change question Further information is requested [Size] Large [Type] Maintenance Cleanup or handling of technical debt.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants