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

Update serialize operations for Gramps 6.0 #617

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

Conversation

dsblank
Copy link
Member

@dsblank dsblank commented Feb 5, 2025

WIP: this is a first draft of updating the serialization code to new Gramps 6.0 data representation.

I don't understand all of the code changed here, and I don't know what kinds of exceptions were caught (and why) and probably the new json_utils will raise different exceptions.

@dsblank
Copy link
Member Author

dsblank commented Feb 5, 2025

In reference to #616

@@ -39,13 +39,14 @@ def get(self, task_id: str):

def serialize_or_str(obj):
try:
return json.dumps(obj)
return object_to_string(obj) # json.dumps(obj)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This module is quite sloppy because it doesn't use any type hints - in this function, obj is not a Gramps object, so there is no need to change it, we can leave json.dumps.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return object_to_string(obj) # json.dumps(obj)
return json.dumps(obj)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the json methods have been replaced with optimized versions. If obj is actually a dict, then you should use dict_to_string (which uses orjson in this case, and is much faster). But anyway, we want to hide the details in these methods as they are all optimized and will always be the best methods of conversion, even if the internal representation changes going forward.

@DavidMStraub
Copy link
Member

There seems to still be a from _future__ import annotations missing somewhere in Gramps where pipes are used in annotations.

@DavidMStraub
Copy link
Member

Found the cause and will submit a PR to Gramps core soon.

@DavidMStraub
Copy link
Member

Fixed in gramps-project/gramps#1930

from http import HTTPStatus

from celery.result import AsyncResult
from flask import abort

from . import ProtectedResource

from gramps.gen.lib.json_utils import object_to_string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
from gramps.gen.lib.json_utils import object_to_string

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably want to change that to dict_to_string

@@ -16,7 +16,8 @@ classifiers = [
]
requires-python = ">=3.9"
dependencies = [
"gramps[GUI,i18n]==5.2.*",
"gramps[GUI,i18n]==6.0.0b1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can try with "gramps[all] @ git+https://github.com/gramps-project/gramps.git@maintenance/gramps60" now that the fixes have been merged into this branch.

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.

2 participants