Skip to content
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

generalize JSON serializer #16733

Merged
merged 1 commit into from
Oct 30, 2023
Merged

generalize JSON serializer #16733

merged 1 commit into from
Oct 30, 2023

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Oct 30, 2023

Purpose:

This makes it work with any type that has a to_json_dict() member, not just dataclasses.

Not making this change was an oversight when introducing the first rust native types among our protocol message types. (here: #11503 ). That PR updates recurse_jsonify() (here: https://github.com/Chia-Network/chia-blockchain/pull/11503/files#diff-dd6f0c93f94df70ed7b9693a16d72d4d79e9f2338eb65b13bebad03b28d9593cR145-R147 ).

This highlights that we currently have two functions to convert a structure to json, recurse_jsonify() as well as the EnhancedJSONEncoder. That seems a bit risky.

@arvidn arvidn added the Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog label Oct 30, 2023
@arvidn arvidn requested a review from altendky October 30, 2023 09:41
@arvidn arvidn marked this pull request as ready for review October 30, 2023 10:33
@arvidn arvidn requested a review from a team as a code owner October 30, 2023 10:33
@arvidn arvidn added the ready_to_merge Submitter and reviewers think this is ready label Oct 30, 2023
@cmmarslender cmmarslender merged commit 1d55fa8 into main Oct 30, 2023
253 of 254 checks passed
@cmmarslender cmmarslender deleted the json-encode branch October 30, 2023 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog ready_to_merge Submitter and reviewers think this is ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants