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

fix: custom Type serialization methods #209

Closed
wants to merge 1 commit into from

Conversation

Rhodanthe1116
Copy link

@Rhodanthe1116 Rhodanthe1116 commented Dec 14, 2024

Summary for #199 and Proposed Solutions

Original Problem

  • The to_json method of Type works as expected, but to_xml does not.
  • For xml attribute ex: <tag time="2012"/> , serialization relies only on to_s or a raw string, which is insufficient for certain use cases (e.g., custom serialization for specific types). Fix specs reqif#1
  • Additionally, the from_{format} methods of Type (e.g., from_xml, from_json) do not function as expected. While this issue isn't directly addressed in this PR, tests have been included to document the problem (currently commented out).

Changes Introduced in This PR

1. Added Specs for Issue #199

  • to_xml and to_json overrides:
    • Created tests for overriding these methods for a simple custom type.
  • HighPrecisionDateTime example:
    • Added an example using Ceramic and HighPrecisionDateTime to demonstrate correct serialization/deserialization as part of the documentation (e.g., in README.md).

2. Applied serialize in Three Areas

  • xml element:
  • xml attribute:
  • json

Considerations and Next Steps

  • Discussion Needed:
    • These changes may affect other parts of the project or introduce unexpected behavior, as I am new to the codebase.
  • Unresolved Issues:
    • The from_{format} methods (e.g., from_xml, from_json) of Type are still not functioning as intended. Tests documenting this issue are included but commented out.

@ronaldtse
Copy link
Contributor

@HassanAkbar I believe this PR is superseded by #224, is that correct? If so please help close this. Thanks!

@HassanAkbar
Copy link
Member

Closing this one in favor of #271.

@HassanAkbar HassanAkbar closed this Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants