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

Refactor JSON serialisation #1787

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Nick-Hall
Copy link
Member

@Nick-Hall Nick-Hall commented Oct 14, 2024

Simplify the JSON serialisation by creating methods to get and set the state of an object.

The method names get_attrs and set_attrs are not ideal and will probably need changing.

There is some copy and paste of these methods from the BaseObject into the Date object. Why are GrampsType, Date, StyledText and StyledTextTag not derived from BaseObject?

TODO: Look at removing the checks for empty dates from the to_json and from_json functions.

@dsblank
Copy link
Member

dsblank commented Oct 15, 2024

Looking very clean!

@dsblank
Copy link
Member

dsblank commented Oct 15, 2024

Why are GrampsType, Date, StyledText and StyledTextTag not derived from BaseObject?

I always wondered that too.

@DavidMStraub
Copy link
Member

By the way, at the moment, the GrampsType values are still dependent on the locale, as we are not using xml_str. It would be nice if we could change that in the next db schema change, so the JSON becomes locale independent.

@Nick-Hall
Copy link
Member Author

Yes. I've already changed GrampsType to store an integer value for pre-defined options and a string for custom values. This now mirrors the properties in the class.

@Nick-Hall
Copy link
Member Author

I've just tried making BaseObject the parent class for GrampsType, Date, StyledText and StyledTextTag. The change to GrampsType causes errors so I'll probably leave it as it is for now.

@Nick-Hall
Copy link
Member Author

Renamed get_attrs and set_attrs to get_object_state and set_object_state.

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

Successfully merging this pull request may close these issues.

3 participants